2016-10-28 08:30:31

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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


2016-10-28 08:31:45

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:33:02

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:33:51

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:34:40

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:36:14

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:37:19

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:38:31

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:39:22

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:40:42

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 08:42:20

by SF Markus Elfring

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2016-10-28 22:32:38

by Jim Davis

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

On Fri, Oct 28, 2016 at 1:31 AM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> 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

2016-10-28 23:42:22

by Jim Davis

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

On Fri, Oct 28, 2016 at 1:40 AM, SF Markus Elfring
<[email protected]> 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

2016-10-30 15:18:20

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

> 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

2016-11-02 15:35:51

by Masahiro Yamada

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

2016-10-29 8:42 GMT+09:00 Jim Davis <[email protected]>:
> On Fri, Oct 28, 2016 at 1:40 AM, SF Markus Elfring
> <[email protected]> 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

2016-11-02 15:42:03

by Masahiro Yamada

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

2016-10-28 17:31 GMT+09:00 SF Markus Elfring <[email protected]>:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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

2016-11-02 17:39:03

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

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

2016-11-02 17:48:16

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

> 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

2016-11-02 18:27:22

by Masahiro Yamada

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

2016-11-03 2:48 GMT+09:00 SF Markus Elfring <[email protected]>:
>> 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

2016-11-02 18:30:14

by Masahiro Yamada

[permalink] [raw]
Subject: Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

2016-11-03 2:38 GMT+09:00 SF Markus Elfring <[email protected]>:
>>>> + 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

2016-11-02 18:46:57

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

> 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

2016-11-03 15:42:29

by Michal Marek

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

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

2016-11-03 15:57:53

by Michal Marek

[permalink] [raw]
Subject: Re: scripts/basic/fixdep: Complete error handling in print_cmdline()

Dne 2.11.2016 v 19:30 Masahiro Yamada napsal(a):
> 2016-11-03 2:38 GMT+09:00 SF Markus Elfring <[email protected]>:
>>>>> + 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

2016-11-03 19:49:51

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

> 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

2016-11-04 12:19:12

by Michal Marek

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

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

2016-11-04 13:49:46

by SF Markus Elfring

[permalink] [raw]
Subject: Re: scripts/basic/bin2c: Complete error handling in main()

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

2017-11-10 18:33:13

by SF Markus Elfring

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

> 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

From 1587144666866159803@xxx Mon Dec 18 17:45:05 +0000 2017
X-GM-THRID: 1583717913885252215
X-Gmail-Labels: Inbox,Category Forums