{"thread":{"id":"47905","subject":"[PATCH] sequencer: factor out strbuf_read_file_or_whine()","startedAt":"2018-02-22T19:29:42Z","lastAt":"2018-02-26T09:04:42Z","messageCount":8,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"339951","messageId":"6b58885c-b0f6-1687-3f2d-4594aacff9ac@web.de","threadId":"47905","inReplyTo":null,"subject":"[PATCH] sequencer: factor out strbuf_read_file_or_whine()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-02-22T19:29:25Z","receivedAt":"2018-02-22T19:29:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Reduce code duplication by factoring out a function that reads an entire\nfile into a strbuf, or reports errors on stderr if something goes wrong.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\nThe difference to using strbuf_read_file() is more detailed error\nmessages for open(2) failures.  But I don't know if we need them -- or\nunder which circumstances reading todo files could fail anyway.  When\ndoing multiple rebases in parallel perhaps?\n\n sequencer.c | 74 +++++++++++++++++++++++--------------------------------------\n 1 file changed, 28 insertions(+), 46 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e9baaf59bd..e34334f0ef 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)\n \treturn count;\n }\n \n+static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n+{\n+\tint fd;\n+\tssize_t len;\n+\n+\tfd = open(path, O_RDONLY);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"could not open '%s'\"), path);\n+\tlen = strbuf_read(sb, fd, 0);\n+\tclose(fd);\n+\tif (len < 0)\n+\t\treturn error(_(\"could not read '%s'.\"), path);\n+\treturn len;\n+}\n+\n static int read_populate_todo(struct todo_list *todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n \tstruct stat st;\n \tconst char *todo_file = get_todo_path(opts);\n-\tint fd, res;\n+\tint res;\n \n \tstrbuf_reset(&todo_list->buf);\n-\tfd = open(todo_file, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n-\tif (strbuf_read(&todo_list->buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n-\t}\n-\tclose(fd);\n+\tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n+\t\treturn -1;\n \n \tres = stat(todo_file, &st);\n \tif (res)\n@@ -3151,20 +3160,13 @@ int check_todo_list(void)\n \tstruct strbuf todo_file = STRBUF_INIT;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tstruct strbuf missing = STRBUF_INIT;\n-\tint advise_to_edit_todo = 0, res = 0, fd, i;\n+\tint advise_to_edit_todo = 0, res = 0, i;\n \n \tstrbuf_addstr(&todo_file, rebase_path_todo());\n-\tfd = open(todo_file.buf, O_RDONLY);\n-\tif (fd < 0) {\n-\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n-\t\tgoto leave_check;\n-\t}\n-\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n+\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {\n+\t\tres = -1;\n \t\tgoto leave_check;\n \t}\n-\tclose(fd);\n \tadvise_to_edit_todo = res =\n \t\tparse_insn_buffer(todo_list.buf.buf, &todo_list);\n \n@@ -3180,17 +3182,10 @@ int check_todo_list(void)\n \n \ttodo_list_release(&todo_list);\n \tstrbuf_addstr(&todo_file, \".backup\");\n-\tfd = open(todo_file.buf, O_RDONLY);\n-\tif (fd < 0) {\n-\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n-\t\tgoto leave_check;\n-\t}\n-\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n+\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {\n+\t\tres = -1;\n \t\tgoto leave_check;\n \t}\n-\tclose(fd);\n \tstrbuf_release(&todo_file);\n \tres = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);\n \n@@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)\n \t}\n \tstrbuf_release(&buf);\n \n-\tfd = open(todo_file, O_RDONLY);\n-\tif (fd < 0) {\n-\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n-\t}\n-\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n-\t}\n-\tclose(fd);\n+\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)\n+\t\treturn -1;\n \tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n \t\ttodo_list_release(&todo_list);\n \t\treturn -1;\n@@ -3370,17 +3358,11 @@ int rearrange_squash(void)\n \tconst char *todo_file = rebase_path_todo();\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tstruct hashmap subject2item;\n-\tint res = 0, rearranged = 0, *next, *tail, fd, i;\n+\tint res = 0, rearranged = 0, *next, *tail, i;\n \tchar **subjects;\n \n-\tfd = open(todo_file, O_RDONLY);\n-\tif (fd < 0)\n-\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n-\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n-\t}\n-\tclose(fd);\n+\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)\n+\t\treturn -1;\n \tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n \t\ttodo_list_release(&todo_list);\n \t\treturn -1;\n-- \n2.16.2\n"},{"id":"339975","messageId":"xmqqlgfkvj1t.fsf@gitster-ct.c.googlers.com","threadId":"47905","inReplyTo":"6b58885c-b0f6-1687-3f2d-4594aacff9ac@web.de","subject":"Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-22T20:57:50Z","receivedAt":"2018-02-22T20:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Reduce code duplication by factoring out a function that reads an entire\n> file into a strbuf, or reports errors on stderr if something goes wrong.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> The difference to using strbuf_read_file() is more detailed error\n> messages for open(2) failures.  But I don't know if we need them -- or\n> under which circumstances reading todo files could fail anyway.  When\n> doing multiple rebases in parallel perhaps?\n>\n>  sequencer.c | 74 +++++++++++++++++++++++--------------------------------------\n>  1 file changed, 28 insertions(+), 46 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index e9baaf59bd..e34334f0ef 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)\n>  \treturn count;\n>  }\n>  \n> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n> +{\n> +\tint fd;\n> +\tssize_t len;\n> +\n> +\tfd = open(path, O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n> +\tlen = strbuf_read(sb, fd, 0);\n> +\tclose(fd);\n> +\tif (len < 0)\n> +\t\treturn error(_(\"could not read '%s'.\"), path);\n> +\treturn len;\n> +}\n> +\n\nThis looks like a good granularity of a unit of independent work.\nThe original we see below looks like it was written with scissors\nand glue ;-)\n\nIt appears to me that no topic in flight introduce more instances\nthat need to be converted with a quick trial merge to 'pu', so I'll\nqueue this forked at the tip of 'master'.\n\nThanks.\n\n>  static int read_populate_todo(struct todo_list *todo_list,\n>  \t\t\tstruct replay_opts *opts)\n>  {\n>  \tstruct stat st;\n>  \tconst char *todo_file = get_todo_path(opts);\n> -\tint fd, res;\n> +\tint res;\n>  \n>  \tstrbuf_reset(&todo_list->buf);\n> -\tfd = open(todo_file, O_RDONLY);\n> -\tif (fd < 0)\n> -\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n> -\tif (strbuf_read(&todo_list->buf, fd, 0) < 0) {\n> -\t\tclose(fd);\n> -\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n> -\t}\n> -\tclose(fd);\n> +\tif (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)\n> +\t\treturn -1;\n>  \n>  \tres = stat(todo_file, &st);\n>  \tif (res)\n> @@ -3151,20 +3160,13 @@ int check_todo_list(void)\n>  \tstruct strbuf todo_file = STRBUF_INIT;\n>  \tstruct todo_list todo_list = TODO_LIST_INIT;\n>  \tstruct strbuf missing = STRBUF_INIT;\n> -\tint advise_to_edit_todo = 0, res = 0, fd, i;\n> +\tint advise_to_edit_todo = 0, res = 0, i;\n>  \n>  \tstrbuf_addstr(&todo_file, rebase_path_todo());\n> -\tfd = open(todo_file.buf, O_RDONLY);\n> -\tif (fd < 0) {\n> -\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n> -\t\tgoto leave_check;\n> -\t}\n> -\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n> -\t\tclose(fd);\n> -\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n> +\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {\n> +\t\tres = -1;\n>  \t\tgoto leave_check;\n>  \t}\n> -\tclose(fd);\n>  \tadvise_to_edit_todo = res =\n>  \t\tparse_insn_buffer(todo_list.buf.buf, &todo_list);\n>  \n> @@ -3180,17 +3182,10 @@ int check_todo_list(void)\n>  \n>  \ttodo_list_release(&todo_list);\n>  \tstrbuf_addstr(&todo_file, \".backup\");\n> -\tfd = open(todo_file.buf, O_RDONLY);\n> -\tif (fd < 0) {\n> -\t\tres = error_errno(_(\"could not open '%s'\"), todo_file.buf);\n> -\t\tgoto leave_check;\n> -\t}\n> -\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n> -\t\tclose(fd);\n> -\t\tres = error(_(\"could not read '%s'.\"), todo_file.buf);\n> +\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file.buf) < 0) {\n> +\t\tres = -1;\n>  \t\tgoto leave_check;\n>  \t}\n> -\tclose(fd);\n>  \tstrbuf_release(&todo_file);\n>  \tres = !!parse_insn_buffer(todo_list.buf.buf, &todo_list);\n>  \n> @@ -3271,15 +3266,8 @@ int skip_unnecessary_picks(void)\n>  \t}\n>  \tstrbuf_release(&buf);\n>  \n> -\tfd = open(todo_file, O_RDONLY);\n> -\tif (fd < 0) {\n> -\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n> -\t}\n> -\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n> -\t\tclose(fd);\n> -\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n> -\t}\n> -\tclose(fd);\n> +\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)\n> +\t\treturn -1;\n>  \tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n>  \t\ttodo_list_release(&todo_list);\n>  \t\treturn -1;\n> @@ -3370,17 +3358,11 @@ int rearrange_squash(void)\n>  \tconst char *todo_file = rebase_path_todo();\n>  \tstruct todo_list todo_list = TODO_LIST_INIT;\n>  \tstruct hashmap subject2item;\n> -\tint res = 0, rearranged = 0, *next, *tail, fd, i;\n> +\tint res = 0, rearranged = 0, *next, *tail, i;\n>  \tchar **subjects;\n>  \n> -\tfd = open(todo_file, O_RDONLY);\n> -\tif (fd < 0)\n> -\t\treturn error_errno(_(\"could not open '%s'\"), todo_file);\n> -\tif (strbuf_read(&todo_list.buf, fd, 0) < 0) {\n> -\t\tclose(fd);\n> -\t\treturn error(_(\"could not read '%s'.\"), todo_file);\n> -\t}\n> -\tclose(fd);\n> +\tif (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0)\n> +\t\treturn -1;\n>  \tif (parse_insn_buffer(todo_list.buf.buf, &todo_list) < 0) {\n>  \t\ttodo_list_release(&todo_list);\n>  \t\treturn -1;\n"},{"id":"339998","messageId":"20180223064952.GB19791@sigill.intra.peff.net","threadId":"47905","inReplyTo":"6b58885c-b0f6-1687-3f2d-4594aacff9ac@web.de","subject":"Re: [PATCH] sequencer: factor out strbuf_read_file_or_whine()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-23T06:49:52Z","receivedAt":"2018-02-23T06:49:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 22, 2018 at 08:29:25PM +0100, René Scharfe wrote:\n\n> Reduce code duplication by factoring out a function that reads an entire\n> file into a strbuf, or reports errors on stderr if something goes wrong.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> The difference to using strbuf_read_file() is more detailed error\n> messages for open(2) failures.  But I don't know if we need them -- or\n> under which circumstances reading todo files could fail anyway.  When\n> doing multiple rebases in parallel perhaps?\n\nI'm fine with this patch, but FWIW I think reporting the result of\nstrbuf_read_file with error_errno() would actually be an improvement.\nThe errno values are generally sufficient to tell if the problem was in\nopening or reading, and then we'd get more information in the case of a\nfailed read() call.\n\nThought note...\n\n> diff --git a/sequencer.c b/sequencer.c\n> index e9baaf59bd..e34334f0ef 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1869,22 +1869,31 @@ static int count_commands(struct todo_list *todo_list)\n>  \treturn count;\n>  }\n>  \n> +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n> +{\n> +\tint fd;\n> +\tssize_t len;\n> +\n> +\tfd = open(path, O_RDONLY);\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n> +\tlen = strbuf_read(sb, fd, 0);\n> +\tclose(fd);\n> +\tif (len < 0)\n> +\t\treturn error(_(\"could not read '%s'.\"), path);\n> +\treturn len;\n> +}\n\nIf we were to use error_errno() in the second conditional here, we\nshould take care not to clobber errno during the close(). I think\nstrbuf_read_file() actually has the same problem, which might be worth\nfixing.\n\n-Peff\n"},{"id":"339999","messageId":"20180223070053.GC19791@sigill.intra.peff.net","threadId":"47905","inReplyTo":"20180223064952.GB19791@sigill.intra.peff.net","subject":"[PATCH] strbuf_read_file(): preserve errno across close() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-23T07:00:54Z","receivedAt":"2018-02-23T07:01:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 23, 2018 at 01:49:52AM -0500, Jeff King wrote:\n\n> > +static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n> > +{\n> > +\tint fd;\n> > +\tssize_t len;\n> > +\n> > +\tfd = open(path, O_RDONLY);\n> > +\tif (fd < 0)\n> > +\t\treturn error_errno(_(\"could not open '%s'\"), path);\n> > +\tlen = strbuf_read(sb, fd, 0);\n> > +\tclose(fd);\n> > +\tif (len < 0)\n> > +\t\treturn error(_(\"could not read '%s'.\"), path);\n> > +\treturn len;\n> > +}\n> \n> If we were to use error_errno() in the second conditional here, we\n> should take care not to clobber errno during the close(). I think\n> strbuf_read_file() actually has the same problem, which might be worth\n> fixing.\n\nHere's a patch, while I'm thinking about it.\n\nI notice that quite a few strbuf error paths may call strbuf_release(),\ntoo.  Technically free() may clobber errno, too. I don't know if it's\nworth protecting against (IIRC POSIX is being amended to disallow this,\nbut I have no idea how common it is in existing platforms).\n\n-- >8 --\nSubject: [PATCH] strbuf_read_file(): preserve errno across close() call\n\nIf we encounter a read error, the user may want to report it\nby looking at errno. However, our close() call may clobber\nerrno, leading to confusing results. Let's save and restore\nit in the error case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n strbuf.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 1df674e919..5f138ed3c8 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -612,14 +612,18 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n {\n \tint fd;\n \tssize_t len;\n+\tint saved_errno;\n \n \tfd = open(path, O_RDONLY);\n \tif (fd < 0)\n \t\treturn -1;\n \tlen = strbuf_read(sb, fd, hint);\n+\tsaved_errno = errno;\n \tclose(fd);\n-\tif (len < 0)\n+\tif (len < 0) {\n+\t\terrno = saved_errno;\n \t\treturn -1;\n+\t}\n \n \treturn len;\n }\n-- \n2.16.2.580.g96c83ce8ea\n\n"},{"id":"340071","messageId":"f134c6bf-c147-4201-1217-b59bfb9f2288@web.de","threadId":"47905","inReplyTo":"20180223070053.GC19791@sigill.intra.peff.net","subject":"Re: [PATCH] strbuf_read_file(): preserve errno across close() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-02-23T21:00:24Z","receivedAt":"2018-02-23T21:00:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.02.2018 um 08:00 schrieb Jeff King:\n> On Fri, Feb 23, 2018 at 01:49:52AM -0500, Jeff King wrote:\n> Subject: [PATCH] strbuf_read_file(): preserve errno across close() call\n> \n> If we encounter a read error, the user may want to report it\n> by looking at errno. However, our close() call may clobber\n> errno, leading to confusing results. Let's save and restore\n> it in the error case.\n\nGood idea.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>   strbuf.c | 6 +++++-\n>   1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index 1df674e919..5f138ed3c8 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -612,14 +612,18 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n>   {\n>   \tint fd;\n>   \tssize_t len;\n> +\tint saved_errno;\n>   \n>   \tfd = open(path, O_RDONLY);\n>   \tif (fd < 0)\n>   \t\treturn -1;\n>   \tlen = strbuf_read(sb, fd, hint);\n> +\tsaved_errno = errno;\n>   \tclose(fd);\n> -\tif (len < 0)\n> +\tif (len < 0) {\n> +\t\terrno = saved_errno;\n>   \t\treturn -1;\n> +\t}\n>   \n>   \treturn len;\n>   }\n\nHow about adding a stealthy close_no_errno(), or do something like the\nfollowing to get shorter and more readable code?  (We could also keep\na single close() call, but would then set errno even on success.)\n\n--- \n strbuf.c | 10 +++++++---\n 1 file changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 1df674e919..c0066b1db9 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -2,6 +2,8 @@\n #include \"refs.h\"\n #include \"utf8.h\"\n \n+#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)\n+\n int starts_with(const char *str, const char *prefix)\n {\n \tfor (; ; str++, prefix++)\n@@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \t\tif (got < 0) {\n \t\t\tif (oldalloc == 0)\n-\t\t\t\tstrbuf_release(sb);\n+\t\t\t\tIGNORE_ERROR(strbuf_release(sb));\n \t\t\telse\n \t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n@@ -617,9 +619,11 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n \tif (fd < 0)\n \t\treturn -1;\n \tlen = strbuf_read(sb, fd, hint);\n-\tclose(fd);\n-\tif (len < 0)\n+\tif (len < 0) {\n+\t\tIGNORE_ERROR(close(fd));\n \t\treturn -1;\n+\t}\n+\tclose(fd);\n \n \treturn len;\n }\n"},{"id":"340082","messageId":"xmqqk1v3s64y.fsf@gitster-ct.c.googlers.com","threadId":"47905","inReplyTo":"f134c6bf-c147-4201-1217-b59bfb9f2288@web.de","subject":"Re: [PATCH] strbuf_read_file(): preserve errno across close() call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-23T22:17:17Z","receivedAt":"2018-02-23T22:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> +#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)\n\nThe macro certainly is a cute idea, but ...\n\n> @@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>  \n>  \t\tif (got < 0) {\n>  \t\t\tif (oldalloc == 0)\n> -\t\t\t\tstrbuf_release(sb);\n> +\t\t\t\tIGNORE_ERROR(strbuf_release(sb));\n>  \t\t\telse\n>  \t\t\t\tstrbuf_setlen(sb, oldlen);\n>  \t\t\treturn -1;\n\n... ideally, I would imagine that we wish we could write this hunk\nto something that expands to:\n\n\t\tif (got < 0) {\n\t\t\tdo {\n                                int e_ = errno;\n                                if (oldalloc == 0)\n                                        strbuf_release(sb);\n                                else\n                                        strbuf_setlen(sb, oldlen);\n                                errno = e_;\n\t\t\t} while (0);\n\t\t\treturn -1;\n\nno?  That is (1) we do not want to rely too much on knowing that\nstrbuf_setlen() is very thin and does not touch errno, and hence (2)\nwe want to mark not just a single expr but a block as \"we know we\ngot an error and errno from that error is more precious than what we\ndo in this block to clean thihngs up\".\n\nOf course, a pair of macros\n\n\t#define IGNORE_ERROR_BEGIN do { int e_ = errno\n\t#define IGNORE_ERROR_END errno = e_; } while (0)\n\nis probably the only way to do so in C, and that is already too ugly\nto live, so we cannot achieve the ideal.\n\nSo I dunno..\n\n> @@ -617,9 +619,11 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n>  \tif (fd < 0)\n>  \t\treturn -1;\n>  \tlen = strbuf_read(sb, fd, hint);\n> -\tclose(fd);\n> -\tif (len < 0)\n> +\tif (len < 0) {\n> +\t\tIGNORE_ERROR(close(fd));\n>  \t\treturn -1;\n> +\t}\n> +\tclose(fd);\n>  \n>  \treturn len;\n>  }\n"},{"id":"340093","messageId":"612cc15c-624c-0963-4d12-aed0993ea16c@web.de","threadId":"47905","inReplyTo":"xmqqk1v3s64y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] strbuf_read_file(): preserve errno across close() call","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2018-02-23T22:55:59Z","receivedAt":"2018-02-23T22:56:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 23.02.2018 um 23:17 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n> \n>> +#define IGNORE_ERROR(expr) do { int e_ = errno; expr; errno = e_; } while (0)\n> \n> The macro certainly is a cute idea, but ...\n> \n>> @@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>>   \n>>   \t\tif (got < 0) {\n>>   \t\t\tif (oldalloc == 0)\n>> -\t\t\t\tstrbuf_release(sb);\n>> +\t\t\t\tIGNORE_ERROR(strbuf_release(sb));\n>>   \t\t\telse\n>>   \t\t\t\tstrbuf_setlen(sb, oldlen);\n>>   \t\t\treturn -1;\n> \n> ... ideally, I would imagine that we wish we could write this hunk\n> to something that expands to:\n> \n> \t\tif (got < 0) {\n> \t\t\tdo {\n>                                  int e_ = errno;\n>                                  if (oldalloc == 0)\n>                                          strbuf_release(sb);\n>                                  else\n>                                          strbuf_setlen(sb, oldlen);\n>                                  errno = e_;\n> \t\t\t} while (0);\n> \t\t\treturn -1;\n> \n> no?  That is (1) we do not want to rely too much on knowing that\n> strbuf_setlen() is very thin and does not touch errno, and hence (2)\n> we want to mark not just a single expr but a block as \"we know we\n> got an error and errno from that error is more precious than what we\n> do in this block to clean thihngs up\".\n\nRelying on that internal knowledge should be OK in strbuf.c, but in\nthis specific example we could of course do:\n\n\t\t\tif (oldalloc == 0)\n\t\t\t\tIGNORE_ERROR(strbuf_release(sb));\n\t\t\telse\n\t\t\t\tIGNORE_ERROR(strbuf_setlen(sb, oldlen));\n\nI guess ignoring errors of whole blocks is not that common, based on\na quick search (git grep -W int.*_errno).  And in such a case we could\nfactor that code out into a separate function, if really needed.  Or\ncontinue saving errno explicitly.\n\nCompilers should be smart enough to avoid saving and restoring errno\nbetween multiple uses of that macro, e.g. code like this would only do\nit once, from what I saw when experimenting with the Compiler Explorer\n(https://godbolt.org/):\n\n\tIGNORE_ERROR(close(fd1));\n\tIGNORE_ERROR(close(fd2));\n\n> Of course, a pair of macros\n> \n> \t#define IGNORE_ERROR_BEGIN do { int e_ = errno\n> \t#define IGNORE_ERROR_END errno = e_; } while (0)\n> \n> is probably the only way to do so in C, and that is already too ugly\n> to live, so we cannot achieve the ideal.\n> \n> So I dunno..\n\n*shudder*\n\n> \n>> @@ -617,9 +619,11 @@ ssize_t strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n>>   \tif (fd < 0)\n>>   \t\treturn -1;\n>>   \tlen = strbuf_read(sb, fd, hint);\n>> -\tclose(fd);\n>> -\tif (len < 0)\n>> +\tif (len < 0) {\n>> +\t\tIGNORE_ERROR(close(fd));\n>>   \t\treturn -1;\n>> +\t}\n>> +\tclose(fd);\n>>   \n>>   \treturn len;\n>>   }\n"},{"id":"340277","messageId":"20180226090433.GB30343@sigill.intra.peff.net","threadId":"47905","inReplyTo":"f134c6bf-c147-4201-1217-b59bfb9f2288@web.de","subject":"Re: [PATCH] strbuf_read_file(): preserve errno across close() call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-26T09:04:33Z","receivedAt":"2018-02-26T09:04:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 23, 2018 at 10:00:24PM +0100, René Scharfe wrote:\n\n> How about adding a stealthy close_no_errno(), or do something like the\n> following to get shorter and more readable code?  (We could also keep\n> a single close() call, but would then set errno even on success.)\n> [...]\n> @@ -391,7 +393,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>  \n>  \t\tif (got < 0) {\n>  \t\t\tif (oldalloc == 0)\n> -\t\t\t\tstrbuf_release(sb);\n> +\t\t\t\tIGNORE_ERROR(strbuf_release(sb));\n>  \t\t\telse\n>  \t\t\t\tstrbuf_setlen(sb, oldlen);\n>  \t\t\treturn -1;\n\nI dunno, that may be crossing the line of \"too magical\".\n\nI had envisioned something like:\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 5f138ed3c8..0790dd7bcb 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -365,6 +365,14 @@ void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)\n \t}\n }\n \n+/* release, but preserve errno */\n+static void strbuf_release_careful(struct strbuf *sb)\n+{\n+\tint saved_errno = errno;\n+\tstrbuf_release(sb);\n+\terrno = saved_errno;\n+}\n+\n size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n {\n \tsize_t res;\n@@ -375,7 +383,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)\n \tif (res > 0)\n \t\tstrbuf_setlen(sb, sb->len + res);\n \telse if (oldalloc == 0)\n-\t\tstrbuf_release(sb);\n+\t\tstrbuf_release_careful(sb);\n \treturn res;\n }\n \n@@ -391,7 +399,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \t\tif (got < 0) {\n \t\t\tif (oldalloc == 0)\n-\t\t\t\tstrbuf_release(sb);\n+\t\t\t\tstrbuf_release_careful(sb);\n \t\t\telse\n \t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n@@ -416,7 +424,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n \tif (cnt > 0)\n \t\tstrbuf_setlen(sb, sb->len + cnt);\n \telse if (oldalloc == 0)\n-\t\tstrbuf_release(sb);\n+\t\tstrbuf_release_careful(sb);\n \treturn cnt;\n }\n \n@@ -482,7 +490,7 @@ int strbuf_getcwd(struct strbuf *sb)\n \t\t\tbreak;\n \t}\n \tif (oldalloc == 0)\n-\t\tstrbuf_release(sb);\n+\t\tstrbuf_release_careful(sb);\n \telse\n \t\tstrbuf_reset(sb);\n \treturn -1;\n\n\nbut that solution is definitely very specific to these cases. I also had\na feeling I should be able to shove the \"oldalloc\" logic into the\nhelper, too, but there are too many different behaviors in the \"else\"\nblock.\n\n-Peff\n"}]}