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

[Ronnie Sahlberg <sahlberg@google.com>]

On Sat, May 17, 2014 at 7:56 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> 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?

Yeah,  but this patch is gone now.
Longer term I think we should get rid of passing errno around.
errno is best to only be checked immediately after a failed system
call and never else.
(otherwise you end up with a hairy forest of save_errno variables.)

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