On Thu, May 28, 2015 at 10:11:55AM -0700, Junio C Hamano wrote:
> > if (contents == MAP_FAILED) {
> > + if (errno == ENODEV && S_ISDIR(st.st_mode))
> > + errno = EISDIR;
> > error("unable to mmap '%s': %s",
> > config_filename, strerror(errno));
> > ret = CONFIG_INVALID_FILE;
>
> I think this patch places the "magic" at the right place, but I
> would have preferred to see something more like this:
>
> if (contents == MAP_FAILED) {
> if (errno == ENODEV && S_ISDIR(st.st_mode))
> error("unable to mmap a directory '%s',
> config_filename);
> else
> error("unable to mmap '%s': %s",
> config_filename, strerror(errno));
> ret = CONFIG_INVALID_FILE;
>
> But that is a very minor preference. I am OK with relying on our
> knowledge that strerror(EISDIR) would give something that says "the
> thing is a directory which is not appropriate for the operation", as
> nobody after that strerror() refers to 'errno' in this codepath.
I am OK if you want to switch it. Certainly EISDIR produces good output
on my system, but I don't know if that is universal.
We also know S_ISDIR(st.st_mode) _before_ we actually mmap. So I was
tempted to simply check it beforehand, under the assumption that the
mmap cannot possibly work if we have a directory. But by doing it in the
error code path, then we _know_ we are not affecting the outcome, only
the error message. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
