Re: [PATCH 4/4] config.c: rewrite ENODEV into EISDIR when mmap fails

From: Jeff King <peff@peff.net>
Date: May 28, 2015, 20:44 UTC
Message-ID: <20150528204436.GB29148@peff.net>
In-Reply-To: <xmqq7frsiq3o.fsf@gitster.dls.corp.google.com>

On Thu, May 28, 2015 at 10:11:55AM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> >  		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