| Message ID | 20220225144245.182659-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fixdep: use fflush() and ferror() to ensure successful write to files | expand |
David answered most of the questions from Nick.
Let me answer this question:
"Why call ferror as opposed to checking the return code of fflush?
Reading the man page closer:"
When fprintf() happens to need to write data to the end device,
the internal buffer is cleared anyway (even if the writing to the
end device fails).
(We do not notice the failure of this because this patch is
removing xprintf().)
If the buffer has been cleared by the previous fprintf() call,
fflush() succeeds because there is no data in the internal buffer.
So, checking the return value of fflush() is not enough.
That's my understanding.
(I ran a small piece of test code under "ulimit -f")
So, we have two options:
[1] Check the return values in *all* the call-sites
of fprintf() and fflush().
[2] Call fprintf() and fflush() or whatever without checking
their return values.
And, check ferror() at the last moment.
If you are really sure that all the return values are checked,
you can go with [1], then you do not need to call ferror(),
but it is generally less fragile than [2].
On Tue, Mar 1, 2022 at 11:28 AM David Laight <David.Laight@aculab.com> wrote:
>
> Someone send HTML mail – outlook is broken – only lets you top post :-(
>
>
>
> The return value from fprintf() is normally the number of bytes written to
>
> the internal buffer (8k in glibc?)
>
> Only if the buffer is full and an actual write() is done do you get any indication of an error.
>
> So you can use the error return from fprintf() to terminate a loop – but it usually
>
> just isn’t worth the effort.
>
> The error status returned by ferror() is ‘sticky’, so you need only check once.
>
> But you need to check before fclose().
>
> Since fclose() has to write out the buffer – that write can also fail.
>
> I’m not sure whether fclose() returns and error in that case, but adding fflush()
>
> makes the coding easier.
>
>
>
> So if you have lots of fprintf() adding data to a file (which is often the case)
>
> almost all of them always succeed – even if the disk is full.
>
> Adding the error paths that can never really happen just makes the
>
> code harder to read and clutters things up.
>
>
>
> David
>
>
>
> From: Nick Desaulniers <ndesaulniers@google.com>
> Sent: 28 February 2022 23:01
> To: Masahiro Yamada <masahiroy@kernel.org>
> Cc: Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>; David Laight <David.Laight@ACULAB.COM>; LKML <linux-kernel@vger.kernel.org>; Michal Marek <michal.lkml@markovi.net>
> Subject: Re: [PATCH v2] fixdep: use fflush() and ferror() to ensure successful write to files
>
>
>
> On Fri, Feb 25, 2022 at 6:43 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Checking the return value of (v)printf does not ensure the successful
> > write to the .cmd file.
>
> Masahiro,
> Apologies for my delay reviewing; I was on vacation last week.
>
> Do you have more context for why this change is necessary? Perhaps you might describe further in the commit message the use case you're trying to support?
>
> Reading the man pages for vprintf(3), fflush(3), and ferror(3), I'm curious why checking the return value of ferror(3) after not doing so for `vprintf` and `fflush` is preferred?
>
> Why not simply unconditionally add a call to fflush while leaving the existing return code checking on vprintf?
>
> >
> > Call fflush() and ferror() to make sure that everything has been
> > written to the file.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > Changes in v2:
> > - add error message
> >
> > scripts/basic/fixdep.c | 46 +++++++++++++++++-------------------------
> > 1 file changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> > index 44e887cff49b..2328f9a641da 100644
> > --- a/scripts/basic/fixdep.c
> > +++ b/scripts/basic/fixdep.c
> > @@ -105,25 +105,6 @@ static void usage(void)
> > exit(1);
> > }
> >
> > -/*
> > - * In the intended usage of this program, the stdout is redirected to .*.cmd
> > - * files. The return value of printf() must be checked to catch any error,
> > - * e.g. "No space left on device".
> > - */
> > -static void xprintf(const char *format, ...)
> > -{
> > - va_list ap;
> > - int ret;
> > -
> > - va_start(ap, format);
> > - ret = vprintf(format, ap);
> > - if (ret < 0) {
> > - perror("fixdep");
> > - exit(1);
>
> Wouldn't the existing approach abort sooner if there was an error encountered?
>
> > - }
> > - va_end(ap);
> > -}
> > -
> > struct item {
> > struct item *next;
> > unsigned int len;
> > @@ -189,7 +170,7 @@ static void use_config(const char *m, int slen)
> >
> > define_config(m, slen, hash);
> > /* Print out a dependency path from a symbol name. */
> > - xprintf(" $(wildcard include/config/%.*s) \\\n", slen, m);
> > + printf(" $(wildcard include/config/%.*s) \\\n", slen, m);
> > }
> >
> > /* test if s ends in sub */
> > @@ -318,13 +299,13 @@ static void parse_dep_file(char *m, const char *target)
> > */
> > if (!saw_any_target) {
> > saw_any_target = 1;
> > - xprintf("source_%s := %s\n\n",
> > - target, m);
> > - xprintf("deps_%s := \\\n", target);
> > + printf("source_%s := %s\n\n",
> > + target, m);
> > + printf("deps_%s := \\\n", target);
> > }
> > is_first_dep = 0;
> > } else {
> > - xprintf(" %s \\\n", m);
> > + printf(" %s \\\n", m);
> > }
> >
> > buf = read_file(m);
> > @@ -347,8 +328,8 @@ static void parse_dep_file(char *m, const char *target)
> > exit(1);
> > }
> >
> > - xprintf("\n%s: $(deps_%s)\n\n", target, target);
> > - xprintf("$(deps_%s):\n", target);
> > + printf("\n%s: $(deps_%s)\n\n", target, target);
> > + printf("$(deps_%s):\n", target);
> > }
> >
> > int main(int argc, char *argv[])
> > @@ -363,11 +344,22 @@ int main(int argc, char *argv[])
> > target = argv[2];
> > cmdline = argv[3];
> >
> > - xprintf("cmd_%s := %s\n\n", target, cmdline);
> > + printf("cmd_%s := %s\n\n", target, cmdline);
> >
> > buf = read_file(depfile);
> > parse_dep_file(buf, target);
> > free(buf);
> >
> > + fflush(stdout);
> > +
> > + /*
> > + * In the intended usage, the stdout is redirected to .*.cmd files.
> > + * Call ferror() to catch errors such as "No space left on device".
> > + */
> > + if (ferror(stdout)) {
>
> Why call ferror as opposed to checking the return code of fflush? Reading the man page closer:
>
> The function feof() tests the end-of-file indicator for the stream pointed to by stream, returning nonzero if it is set. The end-of-file indicator can be cleared only by the function
> clearerr().
>
> The function ferror() tests the error indicator for the stream pointed to by stream, returning nonzero if it is set. The error indicator can be reset only by the clearerr() function.
>
> Does that imply that "the end-of-file indicator" is distinct from "the error indicator?"
>
> > + fprintf(stderr, "fixdep: not all data was written to the output\n");
> > + exit(1);
> > + }
> > +
> > return 0;
> > }
> > --
> > 2.32.0
> >
>
>
>
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
> P Please consider the environment and don't print this e-mail unless you really need to
On Tue, Mar 1, 2022 at 11:28 AM David Laight <David.Laight@aculab.com> wrote: > > Someone send HTML mail – outlook is broken – only lets you top post :-( > > > > The return value from fprintf() is normally the number of bytes written to > > the internal buffer (8k in glibc?) > > Only if the buffer is full and an actual write() is done do you get any indication of an error. > > So you can use the error return from fprintf() to terminate a loop – but it usually > > just isn’t worth the effort. > > The error status returned by ferror() is ‘sticky’, so you need only check once. > > But you need to check before fclose(). > > Since fclose() has to write out the buffer – that write can also fail. > > I’m not sure whether fclose() returns and error in that case, but adding fflush() > > makes the coding easier. I just checked this. fclose() returns -1 if it fails to flush the buffer. [ test code ] #include <stdio.h> int main(void) { char buf[2049]; int ret, i; for (i = 0; i < 2048; i++) buf[i] = 'a'; buf[2048] = 0; ret = printf("%s", buf); fprintf(stderr, "printf() returned: %d\n", ret); ret = fclose(stdout); fprintf(stderr, "fclose() returned %d\n", ret); return 0; } I tested this on Debian buster. I created a very small partition with 1K size, then write data to that partition. root@buster:~# lsblk /dev/vdb1 NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT vdb1 254:17 0 1K 0 part root@buster:~# ./a.out > /dev/vdb1 printf() returned: 2048 fclose() returned -1 The buffer size seems 4k as far as I tested on Debian.
From: Masahiro Yamada > Sent: 01 March 2022 09:06 > > On Tue, Mar 1, 2022 at 11:28 AM David Laight <David.Laight@aculab.com> wrote: > > > > The return value from fprintf() is normally the number of bytes written to > > the internal buffer (8k in glibc?) > > > > Only if the buffer is full and an actual write() is done do you get any indication of an error. > > > > So you can use the error return from fprintf() to terminate a loop – but it usually > > just isn’t worth the effort. > > > > The error status returned by ferror() is ‘sticky’, so you need only check once. > > > > But you need to check before fclose(). > > > > Since fclose() has to write out the buffer – that write can also fail. > > > > I’m not sure whether fclose() returns and error in that case, but adding fflush() > > makes the coding easier. > > > I just checked this. > > fclose() returns -1 if it fails to flush the buffer. But you still need to check ferror() before it. So you might as well do fflush(); if (ferror()); fclose(); Then there is only one error exit path. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Mar 1, 2022 at 12:22 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > David answered most of the questions from Nick. > > > Let me answer this question: > "Why call ferror as opposed to checking the return code of fflush? > Reading the man page closer:" > > > When fprintf() happens to need to write data to the end device, > the internal buffer is cleared anyway (even if the writing to the > end device fails). > (We do not notice the failure of this because this patch is > removing xprintf().) > > > If the buffer has been cleared by the previous fprintf() call, > fflush() succeeds because there is no data in the internal buffer. > > So, checking the return value of fflush() is not enough. > > That's my understanding. > (I ran a small piece of test code under "ulimit -f") > > > So, we have two options: > > [1] Check the return values in *all* the call-sites > of fprintf() and fflush(). > > > [2] Call fprintf() and fflush() or whatever without checking > their return values. > And, check ferror() at the last moment. > > > > > If you are really sure that all the return values are checked, > you can go with [1], then you do not need to call ferror(), > but it is generally less fragile than [2]. Sure, doesn't matter much to me; was more curious "why." Reviewd-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > > On Tue, Mar 1, 2022 at 11:28 AM David Laight <David.Laight@aculab.com> wrote: > > > > Someone send HTML mail – outlook is broken – only lets you top post :-( > > > > > > > > The return value from fprintf() is normally the number of bytes written to > > > > the internal buffer (8k in glibc?) > > > > Only if the buffer is full and an actual write() is done do you get any indication of an error. > > > > So you can use the error return from fprintf() to terminate a loop – but it usually > > > > just isn’t worth the effort. > > > > The error status returned by ferror() is ‘sticky’, so you need only check once. > > > > But you need to check before fclose(). > > > > Since fclose() has to write out the buffer – that write can also fail. > > > > I’m not sure whether fclose() returns and error in that case, but adding fflush() > > > > makes the coding easier. > > > > > > > > So if you have lots of fprintf() adding data to a file (which is often the case) > > > > almost all of them always succeed – even if the disk is full. > > > > Adding the error paths that can never really happen just makes the > > > > code harder to read and clutters things up. > > > > > > > > David > > > > > > > > From: Nick Desaulniers <ndesaulniers@google.com> > > Sent: 28 February 2022 23:01 > > To: Masahiro Yamada <masahiroy@kernel.org> > > Cc: Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>; David Laight <David.Laight@ACULAB.COM>; LKML <linux-kernel@vger.kernel.org>; Michal Marek <michal.lkml@markovi.net> > > Subject: Re: [PATCH v2] fixdep: use fflush() and ferror() to ensure successful write to files > > > > > > > > On Fri, Feb 25, 2022 at 6:43 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > > > Checking the return value of (v)printf does not ensure the successful > > > write to the .cmd file. > > > > Masahiro, > > Apologies for my delay reviewing; I was on vacation last week. > > > > Do you have more context for why this change is necessary? Perhaps you might describe further in the commit message the use case you're trying to support? > > > > Reading the man pages for vprintf(3), fflush(3), and ferror(3), I'm curious why checking the return value of ferror(3) after not doing so for `vprintf` and `fflush` is preferred? > > > > Why not simply unconditionally add a call to fflush while leaving the existing return code checking on vprintf? > > > > > > > > Call fflush() and ferror() to make sure that everything has been > > > written to the file. > > > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > > --- > > > > > > Changes in v2: > > > - add error message > > > > > > scripts/basic/fixdep.c | 46 +++++++++++++++++------------------------- > > > 1 file changed, 19 insertions(+), 27 deletions(-) > > > > > > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c > > > index 44e887cff49b..2328f9a641da 100644 > > > --- a/scripts/basic/fixdep.c > > > +++ b/scripts/basic/fixdep.c > > > @@ -105,25 +105,6 @@ static void usage(void) > > > exit(1); > > > } > > > > > > -/* > > > - * In the intended usage of this program, the stdout is redirected to .*.cmd > > > - * files. The return value of printf() must be checked to catch any error, > > > - * e.g. "No space left on device". > > > - */ > > > -static void xprintf(const char *format, ...) > > > -{ > > > - va_list ap; > > > - int ret; > > > - > > > - va_start(ap, format); > > > - ret = vprintf(format, ap); > > > - if (ret < 0) { > > > - perror("fixdep"); > > > - exit(1); > > > > Wouldn't the existing approach abort sooner if there was an error encountered? > > > > > - } > > > - va_end(ap); > > > -} > > > - > > > struct item { > > > struct item *next; > > > unsigned int len; > > > @@ -189,7 +170,7 @@ static void use_config(const char *m, int slen) > > > > > > define_config(m, slen, hash); > > > /* Print out a dependency path from a symbol name. */ > > > - xprintf(" $(wildcard include/config/%.*s) \\\n", slen, m); > > > + printf(" $(wildcard include/config/%.*s) \\\n", slen, m); > > > } > > > > > > /* test if s ends in sub */ > > > @@ -318,13 +299,13 @@ static void parse_dep_file(char *m, const char *target) > > > */ > > > if (!saw_any_target) { > > > saw_any_target = 1; > > > - xprintf("source_%s := %s\n\n", > > > - target, m); > > > - xprintf("deps_%s := \\\n", target); > > > + printf("source_%s := %s\n\n", > > > + target, m); > > > + printf("deps_%s := \\\n", target); > > > } > > > is_first_dep = 0; > > > } else { > > > - xprintf(" %s \\\n", m); > > > + printf(" %s \\\n", m); > > > } > > > > > > buf = read_file(m); > > > @@ -347,8 +328,8 @@ static void parse_dep_file(char *m, const char *target) > > > exit(1); > > > } > > > > > > - xprintf("\n%s: $(deps_%s)\n\n", target, target); > > > - xprintf("$(deps_%s):\n", target); > > > + printf("\n%s: $(deps_%s)\n\n", target, target); > > > + printf("$(deps_%s):\n", target); > > > } > > > > > > int main(int argc, char *argv[]) > > > @@ -363,11 +344,22 @@ int main(int argc, char *argv[]) > > > target = argv[2]; > > > cmdline = argv[3]; > > > > > > - xprintf("cmd_%s := %s\n\n", target, cmdline); > > > + printf("cmd_%s := %s\n\n", target, cmdline); > > > > > > buf = read_file(depfile); > > > parse_dep_file(buf, target); > > > free(buf); > > > > > > + fflush(stdout); > > > + > > > + /* > > > + * In the intended usage, the stdout is redirected to .*.cmd files. > > > + * Call ferror() to catch errors such as "No space left on device". > > > + */ > > > + if (ferror(stdout)) { > > > > Why call ferror as opposed to checking the return code of fflush? Reading the man page closer: > > > > The function feof() tests the end-of-file indicator for the stream pointed to by stream, returning nonzero if it is set. The end-of-file indicator can be cleared only by the function > > clearerr(). > > > > The function ferror() tests the error indicator for the stream pointed to by stream, returning nonzero if it is set. The error indicator can be reset only by the clearerr() function. > > > > Does that imply that "the end-of-file indicator" is distinct from "the error indicator?" > > > > > + fprintf(stderr, "fixdep: not all data was written to the output\n"); > > > + exit(1); > > > + } > > > + > > > return 0; > > > } > > > -- > > > 2.32.0 > > > > > > > > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > > > P Please consider the environment and don't print this e-mail unless you really need to > > > > -- > Best Regards > Masahiro Yamada
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c index 44e887cff49b..2328f9a641da 100644 --- a/scripts/basic/fixdep.c +++ b/scripts/basic/fixdep.c @@ -105,25 +105,6 @@ static void usage(void) exit(1); } -/* - * In the intended usage of this program, the stdout is redirected to .*.cmd - * files. The return value of printf() must be checked to catch any error, - * e.g. "No space left on device". - */ -static void xprintf(const char *format, ...) -{ - va_list ap; - int ret; - - va_start(ap, format); - ret = vprintf(format, ap); - if (ret < 0) { - perror("fixdep"); - exit(1); - } - va_end(ap); -} - struct item { struct item *next; unsigned int len; @@ -189,7 +170,7 @@ static void use_config(const char *m, int slen) define_config(m, slen, hash); /* Print out a dependency path from a symbol name. */ - xprintf(" $(wildcard include/config/%.*s) \\\n", slen, m); + printf(" $(wildcard include/config/%.*s) \\\n", slen, m); } /* test if s ends in sub */ @@ -318,13 +299,13 @@ static void parse_dep_file(char *m, const char *target) */ if (!saw_any_target) { saw_any_target = 1; - xprintf("source_%s := %s\n\n", - target, m); - xprintf("deps_%s := \\\n", target); + printf("source_%s := %s\n\n", + target, m); + printf("deps_%s := \\\n", target); } is_first_dep = 0; } else { - xprintf(" %s \\\n", m); + printf(" %s \\\n", m); } buf = read_file(m); @@ -347,8 +328,8 @@ static void parse_dep_file(char *m, const char *target) exit(1); } - xprintf("\n%s: $(deps_%s)\n\n", target, target); - xprintf("$(deps_%s):\n", target); + printf("\n%s: $(deps_%s)\n\n", target, target); + printf("$(deps_%s):\n", target); } int main(int argc, char *argv[]) @@ -363,11 +344,22 @@ int main(int argc, char *argv[]) target = argv[2]; cmdline = argv[3]; - xprintf("cmd_%s := %s\n\n", target, cmdline); + printf("cmd_%s := %s\n\n", target, cmdline); buf = read_file(depfile); parse_dep_file(buf, target); free(buf); + fflush(stdout); + + /* + * In the intended usage, the stdout is redirected to .*.cmd files. + * Call ferror() to catch errors such as "No space left on device". + */ + if (ferror(stdout)) { + fprintf(stderr, "fixdep: not all data was written to the output\n"); + exit(1); + } + return 0; }
Checking the return value of (v)printf does not ensure the successful write to the .cmd file. Call fflush() and ferror() to make sure that everything has been written to the file. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- Changes in v2: - add error message scripts/basic/fixdep.c | 46 +++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 27 deletions(-)