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

Message ID 44402655-ef0d-1e2f-0587-f17295a08aa3@users.sourceforge.net
State New
Headers show

Commit Message

SF Markus Elfring Oct. 28, 2016, 8:31 a.m. UTC
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(-)

Comments

Jim Davis Oct. 28, 2016, 10:32 p.m. UTC | #1
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.
Masahiro Yamada Nov. 2, 2016, 3:41 p.m. UTC | #2
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.
SF Markus Elfring Nov. 2, 2016, 5:48 p.m. UTC | #3
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Masahiro Yamada Nov. 2, 2016, 6:26 p.m. UTC | #4
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.
SF Markus Elfring Nov. 2, 2016, 6:46 p.m. UTC | #5
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek Nov. 3, 2016, 3:42 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Nov. 3, 2016, 7:48 p.m. UTC | #7
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek Nov. 4, 2016, 12:19 p.m. UTC | #8
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Nov. 4, 2016, 12:48 p.m. UTC | #9
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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