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

 scripts/basic/fixdep.c | 44 ++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 44e887cff49b..fad6f29373a9 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,20 @@ 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))
+		exit(1);
+
 	return 0;
 }
-- 
2.32.0

From: Masahiro Yamada
> Sent: 21 February 2022 16:43
> To: linux-kbuild@vger.kernel.org
> 
> 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>

Reviewed-by: David Laight <dvid.laight@aculab.com>

I'll note that you've lost the perror("fixdep").
But I suspect that isn't very meaningful.
If the disk is full it'd probably get lost anyway.


> ---
> 
>  scripts/basic/fixdep.c | 44 ++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index 44e887cff49b..fad6f29373a9 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,20 @@ 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))
> +		exit(1);
> +
>  	return 0;
>  }
> --
> 2.32.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Tue, Feb 22, 2022 at 7:33 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Masahiro Yamada
> > Sent: 21 February 2022 16:43
> > To: linux-kbuild@vger.kernel.org
> >
> > 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>
>
> Reviewed-by: David Laight <dvid.laight@aculab.com>
>
> I'll note that you've lost the perror("fixdep").
> But I suspect that isn't very meaningful.
> If the disk is full it'd probably get lost anyway.


perror() will go to stderr, i.e. tty here.
So, that is not the issue.

ferror() itself does not set errno here; "man ferror" says,
"These  functions  should  not  fail  and  do  not set the external
variable errno"

So, I dropped perror() because I am not sure if any related error
message is printed here.

Perhaps, errno was set by some of preceding printf() calls,
but I am not quite sure if it is carried all the way to the end
of this program.








>
> > ---
> >
> >  scripts/basic/fixdep.c | 44 ++++++++++++++++--------------------------
> >  1 file changed, 17 insertions(+), 27 deletions(-)
> >
> > diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> > index 44e887cff49b..fad6f29373a9 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,20 @@ 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))
> > +             exit(1);
> > +
> >       return 0;
> >  }
> > --
> > 2.32.0
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>


--
Best Regards
Masahiro Yamada

From: Masahiro Yamada <masahiroy@kernel.org>
> Sent: 22 February 2022 03:44
> 
> On Tue, Feb 22, 2022 at 7:33 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Masahiro Yamada
> > > Sent: 21 February 2022 16:43
> > > To: linux-kbuild@vger.kernel.org
> > >
> > > 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>
> >
> > Reviewed-by: David Laight <dvid.laight@aculab.com>
> >
> > I'll note that you've lost the perror("fixdep").
> > But I suspect that isn't very meaningful.
> > If the disk is full it'd probably get lost anyway.
> 
> 
> perror() will go to stderr, i.e. tty here.
> So, that is not the issue.
> 
> ferror() itself does not set errno here; "man ferror" says,
> "These  functions  should  not  fail  and  do  not set the external
> variable errno"
> 
> So, I dropped perror() because I am not sure if any related error
> message is printed here.
> 
> Perhaps, errno was set by some of preceding printf() calls,
> but I am not quite sure if it is carried all the way to the end
> of this program.

I was thinking or a slightly more descriptive error message :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)