Re: [PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> In s_update_ref there are two calls that when they fail we return an error
> based on the errno value. In particular we want to return a specific error
> if ENOTDIR happened. Both these functions do have failure modes where they
> may return an error without updating errno, in which case a previous and
> unrelated ENOTDIR may cause us to return the wrong error. Clear errno before
> calling any functions if we check errno afterwards.

If I understand correctly, this is a workaround for some other broken
functions that don't handle errno correctly.  Long-term, wouldn't it be
better to fix the other functions?  In other words, they should change
errno if an only if they return an error status, no?

Of course you are under no obligation to fix the universe, so this
change may be an expedient workaround anyway.  But if you go this route,
then I think a comment would be helpful to explain the unusual clearing
of errno.

Michael

> 
> Also skip initializing a static variable to 0. Statics live in .bss and
> are all automatically initialized to 0.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/fetch.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 55f457c..a93c893 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>  static struct transport *gsecondary;
>  static const char *submodule_prefix = "";
>  static const char *recurse_submodules_default;
> -static int shown_url = 0;
> +static int shown_url;
>  
>  static int option_parse_recurse_submodules(const struct option *opt,
>  				   const char *arg, int unset)
> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>  	if (!rla)
>  		rla = default_rla.buf;
>  	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> +
> +	errno = 0;
>  	lock = lock_any_ref_for_update(ref->name,
>  				       check_old ? ref->old_sha1 : NULL,
>  				       0, NULL);
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/