Marcello Sylvester Bauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
util/ifdtool: add --output flag
Add a optional commandline flag to define the filename of the resulting output file. If this flag is not defined, if will behave like before by creating a new file with a ".new" suffix.
With this additional flag it is not necessary to move the output file at build-time, and the stdout print "Writing new image to <filename>" makes more sense for the build context.
Change-Id: I824e94e93749f55c3576e4ee2f7804d855fefed2 Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io --- M util/ifdtool/ifdtool.c 1 file changed, 30 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38828/1
diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 2bf2f4d..3cc5e15 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -854,17 +854,11 @@
static void write_image(const char *filename, char *image, int size) { - char new_filename[FILENAME_MAX]; // allow long file names int new_fd; - - // - 5: leave room for ".new\0" - strncpy(new_filename, filename, FILENAME_MAX - 5); - strncat(new_filename, ".new", FILENAME_MAX - strlen(filename)); - - printf("Writing new image to %s\n", new_filename); + printf("Writing new image to %s\n", filename);
// Now write out new image - new_fd = open(new_filename, + new_fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (new_fd < 0) { @@ -1429,6 +1423,7 @@ " -x | --extract: extract intel fd modules\n" " -i | --inject <region>:<module> inject file <module> into region <region>\n" " -n | --newlayout <filename> update regions using a flashrom layout file\n" + " -O | --output <filename> output filename\n" " -s | --spifreq <17|20|30|33|48|50> set the SPI frequency\n" " -D | --density <512|1|2|4|8|16|32|64> set chip density (512 in KByte, others in MByte)\n" " -C | --chip <0|1|2> select spi chip on which to operate\n" @@ -1460,6 +1455,7 @@ int mode_altmedisable = 0, altmedisable = 0; char *region_type_string = NULL, *region_fname = NULL; const char *layout_fname = NULL; + char *new_filename = NULL; int region_type = -1, inputfreq = 0; unsigned int new_density = 0; enum spi_frequency spifreq = SPI_FREQUENCY_20MHZ; @@ -1470,6 +1466,7 @@ {"extract", 0, NULL, 'x'}, {"inject", 1, NULL, 'i'}, {"newlayout", 1, NULL, 'n'}, + {"output", 1, NULL, 'O'}, {"spifreq", 1, NULL, 's'}, {"density", 1, NULL, 'D'}, {"chip", 1, NULL, 'C'}, @@ -1484,7 +1481,7 @@ {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:s:p:eluvth?", + while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:O:s:p:eluvth?", long_options, &option_index)) != EOF) { switch (opt) { case 'd': @@ -1543,6 +1540,14 @@ exit(EXIT_FAILURE); } break; + case 'O': + new_filename = strdup(optarg); + if (!new_filename) { + fprintf(stderr, "No output filename specified\n"); + print_usage(argv[0]); + exit(EXIT_FAILURE); + } + break; case 'D': mode_density = 1; new_density = strtoul(optarg, NULL, 0); @@ -1731,6 +1736,14 @@
close(bios_fd);
+ // generate new filename + if (new_filename == NULL) { + new_filename = (char *) malloc((strlen(filename)) * sizeof(char)); + // - 5: leave room for ".new\0" + strcpy(new_filename, filename); + strcat(new_filename, ".new"); + } + check_ifd_version(image, size);
if (mode_dump) @@ -1746,32 +1759,32 @@ validate_layout(image, size);
if (mode_inject) - inject_region(filename, image, size, region_type, + inject_region(new_filename, image, size, region_type, region_fname);
if (mode_newlayout) - new_layout(filename, image, size, layout_fname); + new_layout(new_filename, image, size, layout_fname);
if (mode_spifreq) - set_spi_frequency(filename, image, size, spifreq); + set_spi_frequency(new_filename, image, size, spifreq);
if (mode_density) - set_chipdensity(filename, image, size, new_density); + set_chipdensity(new_filename, image, size, new_density);
if (mode_em100) - set_em100_mode(filename, image, size); + set_em100_mode(new_filename, image, size);
if (mode_locked) - lock_descriptor(filename, image, size); + lock_descriptor(new_filename, image, size);
if (mode_unlocked) - unlock_descriptor(filename, image, size); + unlock_descriptor(new_filename, image, size);
if (mode_altmedisable) { fpsba_t *fpsba = find_fpsba(image, size); fmsba_t *fmsba = find_fmsba(image, size); fpsba_set_altmedisable(fpsba, fmsba, altmedisable); - write_image(filename, image, size); + write_image(new_filename, image, size); }
free(image);
Hello Stefan Reinauer,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38828
to look at the new patch set (#2).
Change subject: util/ifdtool: add --output flag ......................................................................
util/ifdtool: add --output flag
Add an optional commandline flag to define the filename of the resulting output file. If this flag is not defined, it will behave like before by using the old filename with a ".new" suffix.
With this additional flag it is not necessary to move the output file at build-time, and the stdout print "Writing new image to <filename>" makes more sense in the build context.
Change-Id: I824e94e93749f55c3576e4ee2f7804d855fefed2 Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io --- M util/ifdtool/ifdtool.c 1 file changed, 30 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38828/2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
Patch Set 2:
(1 comment)
The new flag will only work for mode_newlayout as the other commands used to operate in place. You have to either copy the file contents first or limit -O to newlayout generation.
https://review.coreboot.org/c/coreboot/+/38828/2/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/38828/2/util/ifdtool/ifdtool.c@1741 PS2, Line 1741: malloc((strlen(filename)) Strlen(Filename) +5
Hello Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38828
to look at the new patch set (#3).
Change subject: util/ifdtool: add --output flag ......................................................................
util/ifdtool: add --output flag
Add an optional commandline flag to define the filename of the resulting output file. If this flag is not defined, it will behave like before by using the old filename with a ".new" suffix.
With this additional flag it is not necessary to move the output file at build-time, and the stdout print "Writing new image to <filename>" makes more sense in the build context.
Change-Id: I824e94e93749f55c3576e4ee2f7804d855fefed2 Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io --- M util/ifdtool/ifdtool.c 1 file changed, 31 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38828/3
Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
Patch Set 3:
(1 comment)
Patch Set 2:
(1 comment)
The new flag will only work for mode_newlayout as the other commands used to operate in place. You have to either copy the file contents first or limit -O to newlayout generation.
All operations uses the write_image() function, which creates a new image with a ".new" suffix.
https://review.coreboot.org/c/coreboot/+/38828/2/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/38828/2/util/ifdtool/ifdtool.c@1741 PS2, Line 1741: malloc((strlen(filename))
Strlen(Filename) +5
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38828/3/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/38828/3/util/ifdtool/ifdtool.c@1741 PS3, Line 1741: new_filename = (char *) malloc((strlen(filename) + 5) * sizeof(char)); check for NULL
Hello Patrick Rudolph, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38828
to look at the new patch set (#4).
Change subject: util/ifdtool: add --output flag ......................................................................
util/ifdtool: add --output flag
Add an optional commandline flag to define the filename of the resulting output file. If this flag is not defined, it will behave like before by using the old filename with a ".new" suffix.
With this additional flag it is not necessary to move the output file at build-time, and the stdout print "Writing new image to <filename>" makes more sense in the build context.
Change-Id: I824e94e93749f55c3576e4ee2f7804d855fefed2 Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io --- M util/ifdtool/ifdtool.c 1 file changed, 35 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/38828/4
Marcello Sylvester Bauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38828/3/util/ifdtool/ifdtool.c File util/ifdtool/ifdtool.c:
https://review.coreboot.org/c/coreboot/+/38828/3/util/ifdtool/ifdtool.c@1741 PS3, Line 1741: new_filename = (char *) malloc((strlen(filename) + 5) * sizeof(char));
check for NULL
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38828 )
Change subject: util/ifdtool: add --output flag ......................................................................
util/ifdtool: add --output flag
Add an optional commandline flag to define the filename of the resulting output file. If this flag is not defined, it will behave like before by using the old filename with a ".new" suffix.
With this additional flag it is not necessary to move the output file at build-time, and the stdout print "Writing new image to <filename>" makes more sense in the build context.
Change-Id: I824e94e93749f55c3576e4ee2f7804d855fefed2 Signed-off-by: Marcello Sylvester Bauer sylv@sylv.io Reviewed-on: https://review.coreboot.org/c/coreboot/+/38828 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org --- M util/ifdtool/ifdtool.c 1 file changed, 35 insertions(+), 17 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved
diff --git a/util/ifdtool/ifdtool.c b/util/ifdtool/ifdtool.c index 84a8ead..97b9cd1 100644 --- a/util/ifdtool/ifdtool.c +++ b/util/ifdtool/ifdtool.c @@ -854,17 +854,11 @@
static void write_image(const char *filename, char *image, int size) { - char new_filename[FILENAME_MAX]; // allow long file names int new_fd; - - // - 5: leave room for ".new\0" - strncpy(new_filename, filename, FILENAME_MAX - 5); - strncat(new_filename, ".new", FILENAME_MAX - strlen(filename)); - - printf("Writing new image to %s\n", new_filename); + printf("Writing new image to %s\n", filename);
// Now write out new image - new_fd = open(new_filename, + new_fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); if (new_fd < 0) { @@ -1429,6 +1423,7 @@ " -x | --extract: extract intel fd modules\n" " -i | --inject <region>:<module> inject file <module> into region <region>\n" " -n | --newlayout <filename> update regions using a flashrom layout file\n" + " -O | --output <filename> output filename\n" " -s | --spifreq <17|20|30|33|48|50> set the SPI frequency\n" " -D | --density <512|1|2|4|8|16|32|64> set chip density (512 in KByte, others in MByte)\n" " -C | --chip <0|1|2> select spi chip on which to operate\n" @@ -1460,6 +1455,7 @@ int mode_altmedisable = 0, altmedisable = 0; char *region_type_string = NULL, *region_fname = NULL; const char *layout_fname = NULL; + char *new_filename = NULL; int region_type = -1, inputfreq = 0; unsigned int new_density = 0; enum spi_frequency spifreq = SPI_FREQUENCY_20MHZ; @@ -1470,6 +1466,7 @@ {"extract", 0, NULL, 'x'}, {"inject", 1, NULL, 'i'}, {"newlayout", 1, NULL, 'n'}, + {"output", 1, NULL, 'O'}, {"spifreq", 1, NULL, 's'}, {"density", 1, NULL, 'D'}, {"chip", 1, NULL, 'C'}, @@ -1484,7 +1481,7 @@ {0, 0, 0, 0} };
- while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:s:p:eluvth?", + while ((opt = getopt_long(argc, argv, "df:D:C:M:xi:n:O:s:p:eluvth?", long_options, &option_index)) != EOF) { switch (opt) { case 'd': @@ -1543,6 +1540,14 @@ exit(EXIT_FAILURE); } break; + case 'O': + new_filename = strdup(optarg); + if (!new_filename) { + fprintf(stderr, "No output filename specified\n"); + print_usage(argv[0]); + exit(EXIT_FAILURE); + } + break; case 'D': mode_density = 1; new_density = strtoul(optarg, NULL, 0); @@ -1731,6 +1736,18 @@
close(bios_fd);
+ // generate new filename + if (new_filename == NULL) { + new_filename = (char *) malloc((strlen(filename) + 5) * sizeof(char)); + if (!new_filename) { + printf("Out of memory.\n"); + exit(EXIT_FAILURE); + } + // - 5: leave room for ".new\0" + strcpy(new_filename, filename); + strcat(new_filename, ".new"); + } + check_ifd_version(image, size);
if (mode_dump) @@ -1746,34 +1763,35 @@ validate_layout(image, size);
if (mode_inject) - inject_region(filename, image, size, region_type, + inject_region(new_filename, image, size, region_type, region_fname);
if (mode_newlayout) - new_layout(filename, image, size, layout_fname); + new_layout(new_filename, image, size, layout_fname);
if (mode_spifreq) - set_spi_frequency(filename, image, size, spifreq); + set_spi_frequency(new_filename, image, size, spifreq);
if (mode_density) - set_chipdensity(filename, image, size, new_density); + set_chipdensity(new_filename, image, size, new_density);
if (mode_em100) - set_em100_mode(filename, image, size); + set_em100_mode(new_filename, image, size);
if (mode_locked) - lock_descriptor(filename, image, size); + lock_descriptor(new_filename, image, size);
if (mode_unlocked) - unlock_descriptor(filename, image, size); + unlock_descriptor(new_filename, image, size);
if (mode_altmedisable) { fpsba_t *fpsba = find_fpsba(image, size); fmsba_t *fmsba = find_fmsba(image, size); fpsba_set_altmedisable(fpsba, fmsba, altmedisable); - write_image(filename, image, size); + write_image(new_filename, image, size); }
+ free(new_filename); free(image);
return 0;
-
Marcello Sylvester Bauer (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)