| Message ID | 44402655-ef0d-1e2f-0587-f17295a08aa3@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
> 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
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.
> 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
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
> 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
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
>>> 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
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; }