Re: [PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it
On Sat, May 17, 2014 at 7:56 AM, Michael Haggerty mhag...@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 sahlb...@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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it
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 sahlb...@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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v10 22/44] fetch.c: clear errno before calling functions that might set it
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. Also skip initializing a static variable to 0. Statics live in .bss and are all automatically initialized to 0. Signed-off-by: Ronnie Sahlberg sahlb...@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); -- 2.0.0.rc3.510.g20c254b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
