* [PATCH] clean: use warning_errno() when appropriate @ 2017-02-13 9:27 Nguyễn Thái Ngọc Duy 2017-02-13 18:34 ` Junio C Hamano 2017-02-14 9:54 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-02-13 9:27 UTC (permalink / raw) To: git; +Cc: Nguyễn Thái Ngọc Duy All these warning() calls are preceded by a system call. Report the actual error to help the user understand why we fail to remove something. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/clean.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/clean.c b/builtin/clean.c index d6bc3aaaea..dc1168747e 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, res = dry_run ? 0 : rmdir(path->buf); if (res) { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; } return res; @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, string_list_append(&dels, quoted.buf); } else { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; ret = 1; } @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, *dir_gone = 1; else { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; ret = 1; } @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) res = dry_run ? 0 : unlink(abs_path.buf); if (res) { qname = quote_path_relative(item->string, NULL, &buf); - warning(_(msg_warn_remove_failed), qname); + warning_errno(_(msg_warn_remove_failed), qname); errors++; } else if (!quiet) { qname = quote_path_relative(item->string, NULL, &buf); -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 9:27 [PATCH] clean: use warning_errno() when appropriate Nguyễn Thái Ngọc Duy @ 2017-02-13 18:34 ` Junio C Hamano 2017-02-13 19:14 ` Jeff King 2017-02-14 9:54 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-02-13 18:34 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > All these warning() calls are preceded by a system call. Report the > actual error to help the user understand why we fail to remove > something. I think this patch is probably correct in the current code, but I say this only after following what quote_path_relative() and relative_path() that is called from it. These warnings are preceded by a call to a system library function, but it is not apparent that they are immediately preceded without anything else that could have failed in between. Side note. There are many calls into strbuf API in these two functions. Any calls to xmalloc() and friends made by strbuf functions may see ENOMEM from underlying malloc() and recover by releasing cached resources, by which time the original errno is unrecoverable. So the above "probably correct" is not strictly true. If we care deeply enough that we want to reliably show the errno we got from the preceding call to a system library function even after whatever comes in between, I think you'd need the usual saved_errno trick. Is that worth it?---I do not offhand have an opinion. > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/clean.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/builtin/clean.c b/builtin/clean.c > index d6bc3aaaea..dc1168747e 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -175,7 +175,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, > res = dry_run ? 0 : rmdir(path->buf); > if (res) { > quote_path_relative(path->buf, prefix, "ed); > - warning(_(msg_warn_remove_failed), quoted.buf); > + warning_errno(_(msg_warn_remove_failed), quoted.buf); > *dir_gone = 0; > } > return res; > @@ -209,7 +209,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, > string_list_append(&dels, quoted.buf); > } else { > quote_path_relative(path->buf, prefix, "ed); > - warning(_(msg_warn_remove_failed), quoted.buf); > + warning_errno(_(msg_warn_remove_failed), quoted.buf); > *dir_gone = 0; > ret = 1; > } > @@ -231,7 +231,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, > *dir_gone = 1; > else { > quote_path_relative(path->buf, prefix, "ed); > - warning(_(msg_warn_remove_failed), quoted.buf); > + warning_errno(_(msg_warn_remove_failed), quoted.buf); > *dir_gone = 0; > ret = 1; > } > @@ -982,7 +982,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) > res = dry_run ? 0 : unlink(abs_path.buf); > if (res) { > qname = quote_path_relative(item->string, NULL, &buf); > - warning(_(msg_warn_remove_failed), qname); > + warning_errno(_(msg_warn_remove_failed), qname); > errors++; > } else if (!quiet) { > qname = quote_path_relative(item->string, NULL, &buf); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 18:34 ` Junio C Hamano @ 2017-02-13 19:14 ` Jeff King 2017-02-13 20:38 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2017-02-13 19:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git On Mon, Feb 13, 2017 at 10:34:06AM -0800, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > All these warning() calls are preceded by a system call. Report the > > actual error to help the user understand why we fail to remove > > something. > > I think this patch is probably correct in the current code, but I > say this only after following what quote_path_relative() and > relative_path() that is called from it. These warnings are preceded > by a call to a system library function, but it is not apparent that > they are immediately preceded without anything else that could have > failed in between. > > Side note. There are many calls into strbuf API in these two > functions. Any calls to xmalloc() and friends made by strbuf > functions may see ENOMEM from underlying malloc() and recover by > releasing cached resources, by which time the original errno is > unrecoverable. So the above "probably correct" is not strictly > true. > > If we care deeply enough that we want to reliably show the errno we > got from the preceding call to a system library function even after > whatever comes in between, I think you'd need the usual saved_errno > trick. Is that worth it?---I do not offhand have an opinion. I wonder if xmalloc() should be the one doing the saved_errno trick. After all, it has only two outcomes: we successfully allocated the memory, or we called die(). And that would transitively make most of the strbuf calls errno-safe (except for obvious syscall-related ones like strbuf_read_file). And in turn that makes quote_path_relative() pretty safe (at least when writing to a strbuf). -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 19:14 ` Jeff King @ 2017-02-13 20:38 ` Junio C Hamano 2017-02-13 21:10 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-02-13 20:38 UTC (permalink / raw) To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git Jeff King <peff@peff.net> writes: > I wonder if xmalloc() should be the one doing the saved_errno trick. > After all, it has only two outcomes: we successfully allocated the > memory, or we called die(). I would be lying if I said I did not considered it when I wrote the message you are responding to, but I rejected it because that would be optimizing for a wrong case, in that most callers of xmalloc() and friends do not do so in the error codepath, and we would be penalizing them by doing the saved_errno dance unconditionally. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 20:38 ` Junio C Hamano @ 2017-02-13 21:10 ` Jeff King 2017-02-13 21:53 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2017-02-13 21:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git On Mon, Feb 13, 2017 at 12:38:47PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I wonder if xmalloc() should be the one doing the saved_errno trick. > > After all, it has only two outcomes: we successfully allocated the > > memory, or we called die(). > > I would be lying if I said I did not considered it when I wrote the > message you are responding to, but I rejected it because that would > be optimizing for a wrong case, in that most callers of xmalloc() > and friends do not do so in the error codepath, and we would be > penalizing them by doing the saved_errno dance unconditionally. Yes, that also occurred to me. I'm not sure if two integer swaps is enough to care about when compared to the cost of a malloc(), though. IOW, I think this may be a case where we should be optimizing for programmer time (fewer lines of code, and one less thing to worry about in the callers) versus squeezing out every instruction. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 21:10 ` Jeff King @ 2017-02-13 21:53 ` Junio C Hamano 2017-02-13 22:22 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-02-13 21:53 UTC (permalink / raw) To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git Jeff King <peff@peff.net> writes: > IOW, I think this may be a case where we should be optimizing for > programmer time (fewer lines of code, and one less thing to worry about > in the callers) versus squeezing out every instruction. Fair enough. Unless we do the save_errno dance in all the helper functions we commonly use to safely stash away errno as necessary and tell developers that they can depend on it, the code in the patch that began this discussion still needs its own saved_errno dance to be safe, though. I do not have a feeling that we are not there yet, even after we teach xmalloc() and its family to do so. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 21:53 ` Junio C Hamano @ 2017-02-13 22:22 ` Jeff King 2017-02-13 23:48 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2017-02-13 22:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy, git On Mon, Feb 13, 2017 at 01:53:33PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > IOW, I think this may be a case where we should be optimizing for > > programmer time (fewer lines of code, and one less thing to worry about > > in the callers) versus squeezing out every instruction. > > Fair enough. > > Unless we do the save_errno dance in all the helper functions we > commonly use to safely stash away errno as necessary and tell > developers that they can depend on it, the code in the patch that > began this discussion still needs its own saved_errno dance to be > safe, though. I do not have a feeling that we are not there yet, > even after we teach xmalloc() and its family to do so. Yeah, I certainly agree that is a potential blocker. Even if it is true today, there's nothing guaranteeing that the quote functions don't grow a new internal detail that violates. So in that sense doing the errno dance as close to the caller who cares is the most _obvious_ thing, even if it isn't the shortest. It would be nice if there was a way to annotate a function as errno-safe, and then transitively compute which other functions were errno-safe when they do not call any errno-unsafe function. I don't know if any static analyzers allow that kind of custom annotation, though (and also I wonder whether the cost/benefit of maintaining those annotations would be worth it). -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] clean: use warning_errno() when appropriate 2017-02-13 22:22 ` Jeff King @ 2017-02-13 23:48 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2017-02-13 23:48 UTC (permalink / raw) To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git Jeff King <peff@peff.net> writes: > So in that sense doing the errno dance as close to the caller who cares > is the most _obvious_ thing, even if it isn't the shortest. Yup. > It would be nice if there was a way to annotate a function as > errno-safe, and then transitively compute which other functions were > errno-safe when they do not call any errno-unsafe function. I don't know > if any static analyzers allow that kind of custom annotation, though > (and also I wonder whether the cost/benefit of maintaining those > annotations would be worth it). Double yup. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] clean: use warning_errno() when appropriate 2017-02-13 9:27 [PATCH] clean: use warning_errno() when appropriate Nguyễn Thái Ngọc Duy 2017-02-13 18:34 ` Junio C Hamano @ 2017-02-14 9:54 ` Nguyễn Thái Ngọc Duy 2017-02-14 18:13 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2017-02-14 9:54 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Nguyễn Thái Ngọc Duy All these warning() calls are preceded by a system call. Report the actual error to help the user understand why we fail to remove something. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- v2 dances with errno builtin/clean.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/builtin/clean.c b/builtin/clean.c index d6bc3aaae..3569736f6 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, struct strbuf quoted = STRBUF_INIT; struct dirent *e; int res = 0, ret = 0, gone = 1, original_len = path->len, len; + int saved_errno; struct string_list dels = STRING_LIST_INIT_DUP; *dir_gone = 1; @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, if (!dir) { /* an empty dir could be removed even if it is unreadble */ res = dry_run ? 0 : rmdir(path->buf); + saved_errno = errno; if (res) { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + errno = saved_errno; + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; } return res; @@ -204,12 +207,14 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, continue; } else { res = dry_run ? 0 : unlink(path->buf); + saved_errno = errno; if (!res) { quote_path_relative(path->buf, prefix, "ed); string_list_append(&dels, quoted.buf); } else { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + errno = saved_errno; + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; ret = 1; } @@ -227,11 +232,13 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, if (*dir_gone) { res = dry_run ? 0 : rmdir(path->buf); + saved_errno = errno; if (!res) *dir_gone = 1; else { quote_path_relative(path->buf, prefix, "ed); - warning(_(msg_warn_remove_failed), quoted.buf); + errno = saved_errno; + warning_errno(_(msg_warn_remove_failed), quoted.buf); *dir_gone = 0; ret = 1; } @@ -853,7 +860,7 @@ static void interactive_main_loop(void) int cmd_clean(int argc, const char **argv, const char *prefix) { - int i, res; + int i, res, saved_errno; int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0; int ignored_only = 0, config_set = 0, errors = 0, gone = 1; int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT; @@ -980,9 +987,11 @@ int cmd_clean(int argc, const char **argv, const char *prefix) } } else { res = dry_run ? 0 : unlink(abs_path.buf); + saved_errno = errno; if (res) { qname = quote_path_relative(item->string, NULL, &buf); - warning(_(msg_warn_remove_failed), qname); + errno = saved_errno; + warning_errno(_(msg_warn_remove_failed), qname); errors++; } else if (!quiet) { qname = quote_path_relative(item->string, NULL, &buf); -- 2.11.0.157.gd943d85 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] clean: use warning_errno() when appropriate 2017-02-14 9:54 ` [PATCH v2] " Nguyễn Thái Ngọc Duy @ 2017-02-14 18:13 ` Junio C Hamano 2017-02-15 0:49 ` Duy Nguyen 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-02-14 18:13 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > All these warning() calls are preceded by a system call. Report the > actual error to help the user understand why we fail to remove > something. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > v2 dances with errno Thanks. > > builtin/clean.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/builtin/clean.c b/builtin/clean.c > index d6bc3aaae..3569736f6 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, > struct strbuf quoted = STRBUF_INIT; > struct dirent *e; > int res = 0, ret = 0, gone = 1, original_len = path->len, len; > + int saved_errno; > struct string_list dels = STRING_LIST_INIT_DUP; > > *dir_gone = 1; > @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, > if (!dir) { > /* an empty dir could be removed even if it is unreadble */ > res = dry_run ? 0 : rmdir(path->buf); > + saved_errno = errno; > if (res) { > quote_path_relative(path->buf, prefix, "ed); I think this part should be more like res = ... : rmdir(...); if (res) { int saved_errno = errno; ... do other things that can touch errno ... errno = saved_errno; ... now we know what the original error was ... The reason to store the errno in saved_errno here is not because we want to help code after "if (res) {...}", but the patch sent as-is gives that impression and is confusing to the readers. Perhaps all hunks of this patch share the same issue? I could locally amend, of course, but I'd like to double check before doing so myself---perhaps you did it this way for a good reason that I am missing? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] clean: use warning_errno() when appropriate 2017-02-14 18:13 ` Junio C Hamano @ 2017-02-15 0:49 ` Duy Nguyen 2017-02-15 1:28 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Duy Nguyen @ 2017-02-15 0:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Jeff King On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > >> All these warning() calls are preceded by a system call. Report the >> actual error to help the user understand why we fail to remove >> something. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> --- >> v2 dances with errno > > Thanks. > >> >> builtin/clean.c | 19 ++++++++++++++----- >> 1 file changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/builtin/clean.c b/builtin/clean.c >> index d6bc3aaae..3569736f6 100644 >> --- a/builtin/clean.c >> +++ b/builtin/clean.c >> @@ -154,6 +154,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, >> struct strbuf quoted = STRBUF_INIT; >> struct dirent *e; >> int res = 0, ret = 0, gone = 1, original_len = path->len, len; >> + int saved_errno; >> struct string_list dels = STRING_LIST_INIT_DUP; >> >> *dir_gone = 1; >> @@ -173,9 +174,11 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag, >> if (!dir) { >> /* an empty dir could be removed even if it is unreadble */ >> res = dry_run ? 0 : rmdir(path->buf); >> + saved_errno = errno; >> if (res) { >> quote_path_relative(path->buf, prefix, "ed); > > I think this part should be more like > > res = ... : rmdir(...); > if (res) { > int saved_errno = errno; > ... do other things that can touch errno ... > errno = saved_errno; > ... now we know what the original error was ... > > The reason to store the errno in saved_errno here is not because we > want to help code after "if (res) {...}", but the patch sent as-is > gives that impression and is confusing to the readers. > > Perhaps all hunks of this patch share the same issue? I could > locally amend, of course, but I'd like to double check before doing > so myself---perhaps you did it this way for a good reason that I am > missing? One thing I like about putting saved_errno right next to the related syscall is, the syscall is visible from the diff (previously some are out of context). This is really minor though. I briefly thought of introducing rmdir_errno() and friends that return -errno on error, so we could do res = ... : rmdir_errno(..); if (res) { errno = -res; warning_errno(...); } But that's more work and the errno = -res is not particularly pleasing to read. I'm fine with moving saved_errno in the error handling blocks. -- Duy ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] clean: use warning_errno() when appropriate 2017-02-15 0:49 ` Duy Nguyen @ 2017-02-15 1:28 ` Junio C Hamano 2017-02-15 1:36 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-02-15 1:28 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Jeff King Duy Nguyen <pclouds@gmail.com> writes: > On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano <gitster@pobox.com> wrote: > ... >> The reason to store the errno in saved_errno here is not because we >> want to help code after "if (res) {...}", but the patch sent as-is >> gives that impression and is confusing to the readers. >> >> Perhaps all hunks of this patch share the same issue? I could >> locally amend, of course, but I'd like to double check before doing >> so myself---perhaps you did it this way for a good reason that I am >> missing? > > One thing I like about putting saved_errno right next to the related > syscall is, the syscall is visible from the diff (previously some are > out of context). This is really minor though. I agree that this is minor. I care more about looking at errno ONLY after we saw our call to a system library function indicated an error, and I wanted to avoid doing: res = dry_run ? 0 : rmdir(path); saved_errno = errno; if (res) { ... do something else ... errno = saved_errno; call_something_that_uses_errno(); When our call to rmdir() did not fail, or when we didn't even call rmdir() at all, what is in errno has nothing to do with what we are doing, and making a copy of it makes the code doubly confusing. Rather, I'd prefer to see: res = dry_run ? 0 : rmdir(path); if (res) { int saved_errno = errno; ... do something else ... errno = saved_errno; call_something_that_uses_errno(); which makes it clear what is going on when reading the resulting code. For now, I'll queue a separate SQUASH??? and wait for others to comment. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] clean: use warning_errno() when appropriate 2017-02-15 1:28 ` Junio C Hamano @ 2017-02-15 1:36 ` Jeff King 0 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2017-02-15 1:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote: > I care more about looking at errno ONLY after we saw our call to a > system library function indicated an error, and I wanted to avoid > doing: > > res = dry_run ? 0 : rmdir(path); > saved_errno = errno; > if (res) { > ... do something else ... > errno = saved_errno; > call_something_that_uses_errno(); > > When our call to rmdir() did not fail, or when we didn't even call > rmdir() at all, what is in errno has nothing to do with what we are > doing, and making a copy of it makes the code doubly confusing. > > Rather, I'd prefer to see: > > res = dry_run ? 0 : rmdir(path); > if (res) { > int saved_errno = errno; > ... do something else ... > errno = saved_errno; > call_something_that_uses_errno(); > > which makes it clear what is going on when reading the resulting > code. > > For now, I'll queue a separate SQUASH??? and wait for others to > comment. I don't have a strong feeling either way, but I think your second example there is probably preferable. The reason to save errno is because of the "something else" that may affect it, and it puts the saving close to that. Duy's version above keeps the saved_errno close to the syscall that caused it, which is nicer for making sure we're saving the right thing, and didn't get fooled by: res = rmdir(path); ... some other stuff ... if (res) { int saved_errno = errno; ... something else ... errno = saved_errno; That's wrong if "some other stuff" touches errno. But I think "saved_errno" is not the right pattern there. It is "stuff away the result _and_ errno for this thing so we can use it later". IOW, I'd expect it to be more like: rmdir_result = rmdir(path); rmdir_errno = errno; ... some other stuff ... if (rmdir_result) show_error(rmdir_errno); And that leads to the "gee, why don't we just encode error values as negative integers" pattern. Which I agree is nicer, but I'm not sure I want to get into wrapping every syscall to give it a better interface. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-02-15 1:36 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-13 9:27 [PATCH] clean: use warning_errno() when appropriate Nguyễn Thái Ngọc Duy 2017-02-13 18:34 ` Junio C Hamano 2017-02-13 19:14 ` Jeff King 2017-02-13 20:38 ` Junio C Hamano 2017-02-13 21:10 ` Jeff King 2017-02-13 21:53 ` Junio C Hamano 2017-02-13 22:22 ` Jeff King 2017-02-13 23:48 ` Junio C Hamano 2017-02-14 9:54 ` [PATCH v2] " Nguyễn Thái Ngọc Duy 2017-02-14 18:13 ` Junio C Hamano 2017-02-15 0:49 ` Duy Nguyen 2017-02-15 1:28 ` Junio C Hamano 2017-02-15 1:36 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).