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

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2015, 17:11 UTC
Message-ID: <xmqq7frsiq3o.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150528080300.GD23395@peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

> If we try to mmap a directory, we'll get ENODEV. This
> translates to "no such device" for the user, which is not
> very helpful. Since we've just fstat()'d the file, we can
> easily check whether the problem was a directory to give a
> better message.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> It feels a bit wrong to put this magic conversion here, and not in
> xmmap. But of course xmmap does not have the stat information.
> ...
> diff --git a/config.c b/config.c
> index e7dc155..29fa012 100644
> --- a/config.c
> +++ b/config.c
> @@ -2056,6 +2056,8 @@ int git_config_set_multivar_in_file(const char *config_filename,
>  		contents = xmmap_gently(NULL, contents_sz, PROT_READ,
>  					MAP_PRIVATE, in_fd, 0);
>  		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.

Thanks.  The patches were a pleasant read.