[PATCH 00/10] scripts/basic: Fine-tuning for seven function implementations

[PATCH 00/10] scripts/basic: Fine-tuning for seven function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 28 Oct 2016 10:18:10 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (10):
  bin2c: Complete error handling in main()
  fixdep: Complete error handling in print_deps()
  fixdep: Use the symbol "MAP_FAILED" in print_deps()
  fixdep: Fix error log output in print_deps()
  fixdep: Complete error handling in parse_dep_file()
  fixdep: Complete error handling in do_config_file()
  fixdep: Fix error log output in do_config_file()
  fixdep: Complete error handling in print_config()
  fixdep: Complete error handling in print_cmdline()
  fixdep: Combine two fprintf() calls into one fputs() call in usage()

 scripts/basic/bin2c.c  |  26 ++++++----
 scripts/basic/fixdep.c | 131 +++++++++++++++++++++++++++++++++----------------
 2 files changed, 105 insertions(+), 52 deletions(-)

-- 
2.10.1

[PATCH 01/10] scripts/basic/bin2c: Complete error handling in main()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 16:15:04 +0200

Return values were not checked from five calls of the function "printf".

This issue was detected also by using the Coccinelle software.


* Add a bit of exception handling there.

* Optimise this function implementation a bit.

  - Replace two output calls with the functions "fputs" and "puts".

  - Use the preincrement operator for the variable "total".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/bin2c.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/scripts/basic/bin2c.c b/scripts/basic/bin2c.c
index c3d7eef..c6c8860 100644
--- a/scripts/basic/bin2c.c
+++ b/scripts/basic/bin2c.c
@@ -8,29 +8,35 @@
  */
 
 #include <stdio.h>
+#include <errno.h>
 
 int main(int argc, char *argv[])
 {
 	int ch, total = 0;
 
 	if (argc > 1)
-		printf("const char %s[] %s=\n",
-			argv[1], argc > 2 ? argv[2] : "");
+		if (printf("const char %s[] %s=\n",
+			   argv[1], argc > 2 ? argv[2] : "") < 16)
+			return errno;
 
 	do {
-		printf("\t\"");
+		if (fputs("\t\"", stdout) < 0)
+			return errno;
+
 		while ((ch = getchar()) != EOF) {
-			total++;
-			printf("\\x%02x", ch);
-			if (total % 16 == 0)
+			if (printf("\\x%02x", ch) < 4)
+				return errno;
+			if (++total % 16 == 0)
 				break;
 		}
-		printf("\"\n");
+
+		if (puts("\"") < 0)
+			return errno;
 	} while (ch != EOF);
 
 	if (argc > 1)
-		printf("\t;\n\n#include <linux/types.h>\n\nconst size_t %s_size = %d;\n",
-		       argv[1], total);
-
+		if (printf("\t;\n\n#include <linux/types.h>\n\nconst size_t %s_size = %d;\n",
+			   argv[1], total) < 54)
+			return errno;
 	return 0;
 }
-- 
2.10.1

[PATCH 02/10] scripts/basic/fixdep: Complete error handling in print_deps()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 18:08:30 +0200

Return values were not checked from calls of the functions "close"
and "munmap".

This issue was detected also by using the Coccinelle software.


Add a bit of exception handling there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index fff818b..c9ce3e3 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -113,6 +113,7 @@
 #include <limits.h>
 #include <ctype.h>
 #include <arpa/inet.h>
+#include <errno.h>
 
 int insert_extra_deps;
 char *target;
@@ -413,21 +414,24 @@ static void print_deps(void)
 	}
 	if (st.st_size == 0) {
 		fprintf(stderr,"fixdep: %s is empty\n",depfile);
-		close(fd);
-		return;
+		goto close_fd;
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	if ((long) map == -1) {
 		perror("fixdep: mmap");
-		close(fd);
-		return;
+		goto close_fd;
 	}
 
 	parse_dep_file(map, st.st_size);
-
-	munmap(map, st.st_size);
-
-	close(fd);
+	if (munmap(map, st.st_size))
+		perror("fixdep: munmap");
+close_fd:
+	if (close(fd)) {
+		int code = errno;
+
+		perror("fixdep: close");
+		exit(code);
+	}
 }
 
 int main(int argc, char *argv[])
-- 
2.10.1

[PATCH 03/10] scripts/basic/fixdep: Use the symbol "MAP_FAILED" in print_deps()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 18:18:54 +0200

Check the return value from a call of the function "mmap" by using
the preprocessor symbol "MAP_FAILED".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index c9ce3e3..0dcec29 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -417,7 +417,7 @@ static void print_deps(void)
 		goto close_fd;
 	}
 	map = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	if ((long) map == -1) {
+	if (map == MAP_FAILED) {
 		perror("fixdep: mmap");
 		goto close_fd;
 	}
-- 
2.10.1

[PATCH 04/10] scripts/basic/fixdep: Fix error log output in print_deps()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 19:04:01 +0200

The function "perror" was called after a call of the function "fprintf"
in two if branches. So it could happen that an error message was displayed
for a failed print operation instead of the failure according to the call
of the function "fstat" or "open" here.

* Pass the relevant error data in the logging calls directly.

* Express that the corresponding return values are intentionally unused
  by casts to void.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 0dcec29..9a2ff68 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -403,13 +403,15 @@ static void print_deps(void)
 
 	fd = open(depfile, O_RDONLY);
 	if (fd < 0) {
-		fprintf(stderr, "fixdep: error opening depfile: ");
-		perror(depfile);
+		(void) fprintf(stderr,
+			       "fixdep: error opening depfile: %s: %s\n",
+			       depfile, strerror(errno));
 		exit(2);
 	}
 	if (fstat(fd, &st) < 0) {
-		fprintf(stderr, "fixdep: error fstat'ing depfile: ");
-		perror(depfile);
+		(void) fprintf(stderr,
+			       "fixdep: error fstat'ing depfile: %s: %s\n",
+			       depfile, strerror(errno));
 		exit(2);
 	}
 	if (st.st_size == 0) {
-- 
2.10.1

[PATCH 05/10] scripts/basic/fixdep: Complete error handling in parse_dep_file()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 19:43:43 +0200

Return values were not checked from five calls of the function "printf".

This issue was detected also by using the Coccinelle software.


* Add a bit of exception handling there.

* Combine these calls into three.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 9a2ff68..5f6a4f4 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -366,14 +366,25 @@ static void parse_dep_file(void *map, size_t len)
 					 */
 					if (!saw_any_target) {
 						saw_any_target = 1;
-						printf("source_%s := %s\n\n",
-							target, s);
-						printf("deps_%s := \\\n",
-							target);
+						if (printf("source_%s := %s\n\n"
+							   "deps_%s := \\\n",
+							   target, s, target)
+						   < 24) {
+							int code = errno;
+
+							perror("fixdep: printf");
+							exit(code);
+						}
 					}
 					is_first_dep = 0;
-				} else
-					printf("  %s \\\n", s);
+				} else {
+					if (printf("  %s \\\n", s) < 5) {
+						int code = errno;
+
+						perror("fixdep: printf");
+						exit(code);
+					}
+				}
 				do_config_file(s);
 			}
 		}
@@ -391,8 +402,13 @@ static void parse_dep_file(void *map, size_t len)
 
 	do_extra_deps();
 
-	printf("\n%s: $(deps_%s)\n\n", target, target);
-	printf("$(deps_%s):\n", target);
+	if (printf("\n%s: $(deps_%s)\n\n"
+		   "$(deps_%s):\n", target, target, target) < 27) {
+		int code = errno;
+
+		perror("fixdep: printf");
+		exit(code);
+	}
 }
 
 static void print_deps(void)
-- 
2.10.1

[PATCH 06/10] scripts/basic/fixdep: Complete error handling in do_config_file()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 22:02:42 +0200

Return values were not checked from four calls of the function "close".

This issue was detected also by using the Coccinelle software.


Add a bit of exception handling there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 5f6a4f4..be0fdaa 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -284,27 +284,33 @@ static void do_config_file(const char *filename)
 		perror(filename);
 		exit(2);
 	}
-	if (st.st_size == 0) {
-		close(fd);
-		return;
-	}
+	if (st.st_size == 0)
+		goto close_fd;
 	map = malloc(st.st_size + 1);
 	if (!map) {
 		perror("fixdep: malloc");
-		close(fd);
-		return;
+		goto close_fd;
 	}
 	if (read(fd, map, st.st_size) != st.st_size) {
 		perror("fixdep: read");
-		close(fd);
-		return;
+		goto close_fd;
 	}
 	map[st.st_size] = '\0';
-	close(fd);
-
+	if (close(fd))
+		goto close_failure;
 	parse_config_file(map);
-
 	free(map);
+	return;
+close_fd:
+	if (close(fd)) {
+close_failure:
+		{
+			int code = errno;
+
+			perror("fixdep: close");
+			exit(code);
+		}
+	}
 }
 
 /*
-- 
2.10.1

[PATCH 07/10] scripts/basic/fixdep: Fix error log output in do_config_file()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 22:15:14 +0200

The function "perror" was called after a call of the function "fprintf"
in two if branches. So it could happen that an error message was displayed
for a failed print operation instead of the failure according to the call
of the function "fstat" or "open" here.

* Pass the relevant error data in the logging calls directly.

* Express that the corresponding return values are intentionally unused
  by casts to void.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index be0fdaa..2c4ec91 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -275,13 +275,15 @@ static void do_config_file(const char *filename)
 
 	fd = open(filename, O_RDONLY);
 	if (fd < 0) {
-		fprintf(stderr, "fixdep: error opening config file: ");
-		perror(filename);
+		(void) fprintf(stderr,
+			       "fixdep: error opening config file: %s: %s\n",
+			       filename, strerror(errno));
 		exit(2);
 	}
 	if (fstat(fd, &st) < 0) {
-		fprintf(stderr, "fixdep: error fstat'ing config file: ");
-		perror(filename);
+		(void) fprintf(stderr,
+			       "fixdep: error fstat'ing config file: %s: %s\n",
+			       filename, strerror(errno));
 		exit(2);
 	}
 	if (st.st_size == 0)
-- 
2.10.1

[PATCH 08/10] scripts/basic/fixdep: Complete error handling in print_config()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 27 Oct 2016 22:45:03 +0200

Return values were not checked from calls of the function "printf"
and "putchar".

This issue was detected also by using the Coccinelle software.

* Add a bit of exception handling there.

* Optimise this function implementation a bit by replacing two output calls
  with the functions "fputs" and "puts".

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 2c4ec91..f5ff6eea 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -142,16 +142,26 @@ static void print_config(const char *m, int slen)
 {
 	int c, i;
 
-	printf("    $(wildcard include/config/");
+	if (fputs("    $(wildcard include/config/", stdout) < 0)
+		goto put_failure;
 	for (i = 0; i < slen; i++) {
 		c = m[i];
 		if (c == '_')
 			c = '/';
 		else
 			c = tolower(c);
-		putchar(c);
+		if (putchar(c) == EOF)
+			goto put_failure;
+	}
+	if (puts(".h) \\") < 0) {
+put_failure:
+		{
+			int code = errno;
+
+			perror("fixdep: print_config");
+			exit(code);
+		}
 	}
-	printf(".h) \\\n");
 }
 
 static void do_extra_deps(void)
-- 
2.10.1

[PATCH 09/10] scripts/basic/fixdep: Complete error handling in print_cmdline()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 28 Oct 2016 09:29:59 +0200

A return value was not checked from a call of the function "printf".

This issue was detected also by using the Coccinelle software.


Add a bit of exception handling there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index f5ff6eea..911347a 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -132,7 +132,12 @@ static void usage(void)
  */
 static void print_cmdline(void)
 {
-	printf("cmd_%s := %s\n\n", target, cmdline);
+	if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {
+		int code = errno;
+
+		perror("fixdep: print_cmdline");
+		exit(code);
+	}
 }
 
 /*
-- 
2.10.1

[PATCH 10/10] scripts/basic/fixdep: Combine two fprintf() calls into one fputs() call in usage()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 28 Oct 2016 09:45:30 +0200

Some data were printed by two separate function calls.
Print the same data by a single call of the function "fputs" instead.

This issue was detected also by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 scripts/basic/fixdep.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 911347a..bdd031f 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -122,8 +122,10 @@ char *cmdline;
 
 static void usage(void)
 {
-	fprintf(stderr, "Usage: fixdep [-e] <depfile> <target> <cmdline>\n");
-	fprintf(stderr, " -e  insert extra dependencies given on stdin\n");
+	if (fputs("Usage: fixdep [-e] <depfile> <target> <cmdline>\n"
+		  " -e  insert extra dependencies given on stdin\n", stderr)
+	   < 0)
+		perror("fixdep: usage");
 	exit(1);
 }
 
-- 
2.10.1

Re: [PATCH 01/10] scripts/basic/bin2c: Complete error handling in main()

[Jim Davis]

On Fri, Oct 28, 2016 at 1:31 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 27 Oct 2016 16:15:04 +0200
>
> Return values were not checked from five calls of the function "printf".
>
> This issue was detected also by using the Coccinelle software.
>
>
> * Add a bit of exception handling there.
>
> * Optimise this function implementation a bit.

The most interesting thing about this patch was trying to figure out
how to actually get bin2c to run at all.  Making a defconfig kernel
didn't run it.  Making a kernel with the latest Ubuntu 16.10 config
file didn't run it.  Setting CONFIG_IKCONFIG runs it (once), for the
folks who want to use scripts/extract-ikconfig.  After that, if you
dig about in the makefiles, it looks like you have to turn on the
Tomoyo LSM -- which doesn't seem to be a common occurrence -- or else
set CONFIG_KEXEC_FILE to generate the 'purgatory' thing it uses.
Again, not the most frequent of events, as far as I can tell.

Given how uncommon running bin2c seems to be, "optimizing" it may not
be a useful project.

-- 
Jim

Re: [PATCH 09/10] scripts/basic/fixdep: Complete error handling in print_cmdline()

[Jim Davis]

On Fri, Oct 28, 2016 at 1:40 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:

> +       if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {

Rather than scatter fragile magic numbers, like 10, throughout the
code, if you're hell-bent on checking for printf errors you could
write a little wrapper function that hid the magic number and bundled
up the errno stuff.

But what would you expect printf error checking to tell a user?
Perhaps that he or she ran out of disk space, but that's going to be
painfully obvious anyway in almost every case.

-- 
Jim

Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

[SF Markus Elfring]

> But what would you expect printf error checking to tell a user?

I wonder that the error detection and corresponding exception handling
was not corrected for the affected source files of build-time tools so far.
https://www.securecoding.cert.org/confluence/display/c/EXP12-C.+Do+not+ignore+values+returned+by+functions

Regards,
Markus

Re: [PATCH 09/10] scripts/basic/fixdep: Complete error handling in print_cmdline()

[Masahiro Yamada]

2016-10-29 8:42 GMT+09:00 Jim Davis <jim.epost@gmail.com>:
> On Fri, Oct 28, 2016 at 1:40 AM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
>
>> +       if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {
>
> Rather than scatter fragile magic numbers, like 10, throughout the
> code, if you're hell-bent on checking for printf errors you could
> write a little wrapper function that hid the magic number and bundled
> up the errno stuff.


BTW, how the magic number "10" was calculated?




-- 
Best Regards
Masahiro Yamada

Re: [PATCH 01/10] scripts/basic/bin2c: Complete error handling in main()

[Masahiro Yamada]

2016-10-28 17:31 GMT+09:00 SF Markus Elfring <elfring@users.sourceforge.net>:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 27 Oct 2016 16:15:04 +0200
>
> Return values were not checked from five calls of the function "printf".
>
> This issue was detected also by using the Coccinelle software.
>
>
> * Add a bit of exception handling there.
>
> * Optimise this function implementation a bit.
>
>   - Replace two output calls with the functions "fputs" and "puts".
>
>   - Use the preincrement operator for the variable "total".
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  scripts/basic/bin2c.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/basic/bin2c.c b/scripts/basic/bin2c.c
> index c3d7eef..c6c8860 100644
> --- a/scripts/basic/bin2c.c
> +++ b/scripts/basic/bin2c.c
> @@ -8,29 +8,35 @@
>   */
>
>  #include <stdio.h>
> +#include <errno.h>
>
>  int main(int argc, char *argv[])
>  {
>         int ch, total = 0;
>
>         if (argc > 1)
> -               printf("const char %s[] %s=\n",
> -                       argv[1], argc > 2 ? argv[2] : "");
> +               if (printf("const char %s[] %s=\n",
> +                          argv[1], argc > 2 ? argv[2] : "") < 16)
> +                       return errno;
>
>         do {
> -               printf("\t\"");
> +               if (fputs("\t\"", stdout) < 0)
> +                       return errno;
> +
>                 while ((ch = getchar()) != EOF) {
> -                       total++;
> -                       printf("\\x%02x", ch);
> -                       if (total % 16 == 0)
> +                       if (printf("\\x%02x", ch) < 4)
> +                               return errno;
> +                       if (++total % 16 == 0)
>                                 break;
>                 }
> -               printf("\"\n");
> +
> +               if (puts("\"") < 0)
> +                       return errno;


Is replacing printf("\"\n") with puts("\"") optimization?


Frankly, the result of this patch
seems extremely unreadable code.


-- 
Best Regards
Masahiro Yamada

Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

[SF Markus Elfring]

>>> +       if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {
>>
>> Rather than scatter fragile magic numbers, like 10, throughout the
>> code, if you're hell-bent on checking for printf errors you could
>> write a little wrapper function that hid the magic number and bundled
>> up the errno stuff.
> 
> 
> BTW, how the magic number "10" was calculated?

Does the passed format string indicate how many characters should be
printed at least?

Regards,
Markus

Re: scripts/basic/bin2c: Complete error handling in main()

[SF Markus Elfring]

> Is replacing printf("\"\n") with puts("\"") optimization?

Is the difference relevant if an “ordinary” string is passed instead of
a format string?


> Frankly, the result of this patch seems extremely unreadable code.

Do you care for more complete error detection and corresponding exception handling
in this source file?

Regards,
Markus

Re: scripts/basic/bin2c: Complete error handling in main()

[Masahiro Yamada]

2016-11-03 2:48 GMT+09:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>> Is replacing printf("\"\n") with puts("\"") optimization?
>
> Is the difference relevant if an “ordinary” string is passed instead of
> a format string?

I think GCC does the replacement automatically
unless -ffreestanding option is given.


With a quick test, I got the following disassembly

0000000000400440 <main>:
  400440: 48 83 ec 08           sub    $0x8,%rsp
  400444: bf c4 05 40 00       mov    $0x4005c4,%edi
  400449: e8 c2 ff ff ff       callq  400410 <puts@plt>
  40044e: 31 c0                 xor    %eax,%eax
  400450: 48 83 c4 08           add    $0x8,%rsp
  400454: c3                   retq

from the following program:

int main(void)
{
    printf("hello, world\n");
    return 0;
}




>
>> Frankly, the result of this patch seems extremely unreadable code.
>
> Do you care for more complete error detection and corresponding exception handling
> in this source file?


I like the code as is.

Such error checks and magic numbers are messy.



-- 
Best Regards
Masahiro Yamada

Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

[Masahiro Yamada]

2016-11-03 2:38 GMT+09:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>>> +       if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {
>>>
>>> Rather than scatter fragile magic numbers, like 10, throughout the
>>> code, if you're hell-bent on checking for printf errors you could
>>> write a little wrapper function that hid the magic number and bundled
>>> up the errno stuff.
>>
>>
>> BTW, how the magic number "10" was calculated?
>
> Does the passed format string indicate how many characters should be
> printed at least?

So, the check is a bit compromised.
The printf() should print at lease 10 characters.
If "target" or "cmdline" is not NULL, it should print more.




-- 
Best Regards
Masahiro Yamada

Re: scripts/basic/bin2c: Complete error handling in main()

[SF Markus Elfring]

> I like the code as is.

Do you really prefer to ignore important return values in the discussed function?


> Such error checks and magic numbers are messy.

Is it safer to detect exceptional situations as early as possible here?

Regards,
Markus

Re: scripts/basic/bin2c: Complete error handling in main()

[Michal Marek]

Dne 2.11.2016 v 19:46 SF Markus Elfring napsal(a):
>> I like the code as is.
> 
> Do you really prefer to ignore important return values in the discussed function?

You could define an xprintf() macro that checks if the return value is <
0 and simply calls perror() and exit(1) in such case.

Michal

Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

[Michal Marek]

Dne 2.11.2016 v 19:30 Masahiro Yamada napsal(a):
> 2016-11-03 2:38 GMT+09:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>>>> +       if (printf("cmd_%s := %s\n\n", target, cmdline) < 10) {
>>>>
>>>> Rather than scatter fragile magic numbers, like 10, throughout the
>>>> code, if you're hell-bent on checking for printf errors you could
>>>> write a little wrapper function that hid the magic number and bundled
>>>> up the errno stuff.
>>>
>>>
>>> BTW, how the magic number "10" was calculated?
>>
>> Does the passed format string indicate how many characters should be
>> printed at least?
> 
> So, the check is a bit compromised.
> The printf() should print at lease 10 characters.
> If "target" or "cmdline" is not NULL, it should print more.

printf() / fprintf() return a negative value if an error such as ENOSPC
occurs. So just check for < 0 and preferably use a wrapper.

Michal

Re: scripts/basic/bin2c: Complete error handling in main()

[SF Markus Elfring]

> You could define an xprintf() macro that checks if the return value
> is < 0 and simply calls perror() and exit(1) in such case.

Does such a macro belong to any general header file from the Linux
software library?

Regards,
Markus

Re: scripts/basic/bin2c: Complete error handling in main()

[Michal Marek]

On 2016-11-03 20:48, SF Markus Elfring wrote:
>> You could define an xprintf() macro that checks if the return value
>> is < 0 and simply calls perror() and exit(1) in such case.
> 
> Does such a macro belong to any general header file from the Linux
> software library?

No.

Michal

Re: scripts/basic/bin2c: Complete error handling in main()

[SF Markus Elfring]

>>> You could define an xprintf() macro that checks if the return value
>>> is < 0 and simply calls perror() and exit(1) in such case.
>> Does such a macro belong to any general header file from the Linux
>> software library?
> No.

Would you like to add it?

How do you think about to reuse it in more source files?

Regards,
Markus

Re: [PATCH 00/10] scripts/basic: Fine-tuning for seven function implementations

[SF Markus Elfring]

> Date: Fri, 28 Oct 2016 10:18:10 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (10):
>   bin2c: Complete error handling in main()
>   fixdep: Complete error handling in print_deps()
>   fixdep: Use the symbol "MAP_FAILED" in print_deps()
>   fixdep: Fix error log output in print_deps()
>   fixdep: Complete error handling in parse_dep_file()
>   fixdep: Complete error handling in do_config_file()
>   fixdep: Fix error log output in do_config_file()
>   fixdep: Complete error handling in print_config()
>   fixdep: Complete error handling in print_cmdline()
>   fixdep: Combine two fprintf() calls into one fputs() call in usage()
> 
>  scripts/basic/bin2c.c  |  26 ++++++----
>  scripts/basic/fixdep.c | 131 +++++++++++++++++++++++++++++++++----------------
>  2 files changed, 105 insertions(+), 52 deletions(-)

Are these update suggestions worth for another look?
https://lkml.org/lkml/2016/10/28/138

Regards,
Markus