Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions

From: Jeff King <peff@peff.net>
Date: Apr 11, 2017, 16:16 UTC
Message-ID: <20170411161617.fyu5pmzgplscoozz@sigill.intra.peff.net>
In-Reply-To: <20170407120354.17736-4-benpeart@microsoft.com>

On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:

Show 10 quoted lines

> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>  done:
>  	sigchain_pop(SIGPIPE);
>  
> -	if (err || errno == EPIPE) {
> +	if (err || errno == EPIPE)
> +		err = err ? err : errno;
> +
> +	return err;
> +}

This isn't a new problem introduced by your patch, but this use of errno seems funny to me. Specifically:

  1. Do we need to save errno before calling sigchain_pop()? It's making
     syscalls (though admittedly they are unlikely to fail).

  2. If err is 0, then nothing failed. Who would have set errno? Aren't
     we reading whatever cruft happened to be in errno before the
     function started?

     So I'm confused about what case would trigger on this errno check
     at all.

Also, I'm not quite sure what the return value of the function is supposed to be; usually we'd use 0 for success and negative for errors. But here we might return a negative value that we got from the packet_* functions, or we might return EPIPE, which is positive. I don't think it's a huge deal because the caller checks "if (err)", but perhaps it should be "-errno" for consistency.

-Peff