Asami Doi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
lib: Call extract() only when initrd.size is more than 0
This CL avoid the fail that happens when initrd.size is 0. extract() returns false even though initrd.size is already 0. In the case of size is 0, we don't know extract it, so call extract() only when initrd.size is more than 0.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d --- M src/lib/fit_payload.c 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/1
diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index a4d3705..57cd4a5 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -246,12 +246,13 @@ /* Repack FDT for handoff to kernel */ pack_fdt(&fdt, dt);
- if (config->ramdisk && - extract(&initrd, config->ramdisk)) { - printk(BIOS_ERR, "ERROR: Failed to extract initrd\n"); - prog_set_entry(payload, NULL, NULL); - rdev_munmap(prog_rdev(payload), data); - return; + if (config->ramdisk && initrd.size > 0) { + if (extract(&initrd, config->ramdisk)) { + printk(BIOS_ERR, "ERROR: Failed to extract initrd\n"); + prog_set_entry(payload, NULL, NULL); + rdev_munmap(prog_rdev(payload), data); + return; + } }
timestamp_add_now(TS_KERNEL_DECOMPRESSION);
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
Patch Set 1:
This CL just adds one if-statement to avoid failing when initrd.size is 0. Could you review this CL?
Hello Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#2).
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
lib: Call extract() only when initrd.size is more than 0
This CL avoid the fail that happens when initrd.size is 0. extract() returns false even though initrd.size is already 0. In the case of size is 0, we don't know extract it, so call extract() only when initrd.size is more than 0.
Signed-off-by: Asami Doi d0iasm.pub@gmail.com Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d --- M src/lib/fit_payload.c 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@9 PS2, Line 9: avoid avoids
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@11 PS2, Line 11: don't know extract Do you mean: don’t know how to?
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@15 PS2, Line 15: Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Please switch the lines, so the Signed-off-by line is at the end.
https://review.coreboot.org/c/coreboot/+/34535/2/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/2/src/lib/fit_payload.c@249 PS2, Line 249: initrd.size > 0 Please make that a separate check, so that you can print out a warning in this case. If `config->ramdisk` is true, then it is likely a user error, and a warning should be shown.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
Patch Set 2:
This seems like a bug somewhere else, I'm not convinced we should handle this. Either you want an initrd and it will have a non-zero size, or you don't want an initrd and config->ramdisk shouldn't be set at all. A FIT image that includes a zero-length initrd sounds invalid to me.
Hello Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#3).
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
lib: Call extract() only when initrd.size is more than 0
This CL avoids the fail that happens when initrd.size is 0. extract() returns false even though initrd.size is already 0. In the case of size is 0, we don't have to extract it. Call extract() only when initrd.size is more than 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 7 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c@249 PS3, Line 249: size I don't like that this will silently fail if ramdisk is present, but the size is zero. That's an invalid combination, so it should be an error. Print out an error saying that initrd size is zero.
Hello Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#4).
Change subject: lib: Call extract() only when initrd.size is more than 0 ......................................................................
lib: Call extract() only when initrd.size is more than 0
This CL avoids the fail that happens when initrd.size is 0. extract() returns false even though initrd.size is already 0. In the case of size is 0, we don't have to extract it. Call extract() only when initrd.size is more than 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/4
Hello Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#5).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/5
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 5:
(4 comments)
I changed the purpose of this CL from avoiding a fail to throwing an error when initrd size is 0 even though ramdisk is present.
I guess there is something wrong in the integrated LinuxBoot so I will investigate it.
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@9 PS2, Line 9: avoid
avoids
Done
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@11 PS2, Line 11: don't know extract
Do you mean: don’t know how to?
Sorry, this part doesn't make sense at all. I updated the description.
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@15 PS2, Line 15: Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d
Please switch the lines, so the Signed-off-by line is at the end.
Done
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c@249 PS3, Line 249: size
I don't like that this will silently fail if ramdisk is present, but the size is zero. […]
Ok, I added new if-statement to throw an error when the initrd size is 0 and updated the description in this CL.
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 5:
This is just additional information. I checked the size for the following output files in payloads/external/LinuxBoot/linuxboot. The initramfs size is already 0.
$ du -h uImage vmlinux.bin.lzma target.dtb initramfs initramfs_u-root.cpio 5.6M uImage // payload 4.6M vmlinux.bin.lzma // compressed kernel 1.0M target.dtb 0 initramfs 8.7M initramfs_u-root.cpio
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c@250 PS5, Line 250: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n"); Why not put the check in extract() instead?
Hello Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#6).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/6
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c@250 PS5, Line 250: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n");
Why not put the check in extract() instead?
Checking the size in extract() is better. I moved it to extract(). Thanks.
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#7).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/7
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@56 PS7, Line 56: Shouldn't this return true?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@55 PS7, Line 55: initrd This should not hardcode initrd, use node->name.
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@56 PS7, Line 56:
Shouldn't this return true?
Either true or false, depending on whether we want it to fail for visibility or continue with a warning to still try to boot without it if possible. But yeah, it should return something.
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#8).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/8
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#9).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/9
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@55 PS7, Line 55: initrd
This should not hardcode initrd, use node->name.
Done
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@56 PS7, Line 56:
Either true or false, depending on whether we want it to fail for visibility or continue with a warn […]
I update to return true when the size is 0 because I believe that an empty kernel/initrd is not our intention.
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#10).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/10
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@54 PS10, Line 54: region Why region and not node?
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@78 PS10, Line 78: memcpy(dst, node->data, node->size); This could result in a buffer overflow if region->size < node->size. Refactoring the code like so will fix the overflow and print the error on line 96: true_size = min(region->size, node->size); memcpy(dst, node->data, true_size);
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 10: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@78 PS10, Line 78: memcpy(dst, node->data, node->size);
This could result in a buffer overflow if region->size < node->size. […]
I don't see how this would result in an error? true_size would still be non-zero, even if you didn't copy enough.
It shouldn't really be possible for region->size to be smaller than node->size here. If you want a check, I'd just use assert().
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#11).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/11
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@54 PS10, Line 54: region
Why region and not node?
I replaced with node. I believe the result doesn't change in both cases to use region->size and node->size, but using node->size is easier to understand.
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@78 PS10, Line 78: memcpy(dst, node->data, node->size);
I don't see how this would result in an error? true_size would still be non-zero, even if you didn't […]
I think the situation "region->size < node->size" never happens in the current implementation. It's possible that region->size is larger than node->size in unpack_fdt() and otherwise the sizes are same in fit_payload().
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#12).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 8 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/12
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c@255 PS12, Line 255: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n"); Why is this needed?
Hello Julius Werner, Patrick Rudolph, build bot (Jenkins), Raul Rangel, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34535
to look at the new patch set (#13).
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com --- M src/lib/fit_payload.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34535/13
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c@255 PS12, Line 255: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n");
Why is this needed?
This was my mistake happened during rebasing. I deleted them. Thank you.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
Maybe send a follow-up to use cb_err struct.
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success. Well that is confusing. ;-)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
Please make sure that all comments are addressed and marked as resolved so this an be merged.
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success.
Well that is confusing. […]
Agreed.
I added a comment about it here: https://review.coreboot.org/c/coreboot/+/25019/37/src/lib/fit_payload.c#45
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success.
Agreed. […]
Why? It exactly describes the functions behavior.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success.
Why? It exactly describes the functions behavior.
Sure, but if i wrote a function that returned 43 on failure and 997 on success, it's not what's expected, and may deserve more of a comment than just the description of WHAT it does. Maybe say WHY it was done that way so other people can understand why you didn't do the conventional thing.
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success.
Sure, but if i wrote a function that returned 43 on failure and 997 on success, it's not what's expe […]
I can add a comment in this CL if it's needed.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/13/src/lib/fit_payload.c@46 PS13, Line 46: * Returns true on error, false on success.
I can add a comment in this CL if it's needed.
Let's keep that discussion in the other patch and address it separately, it has nothing to do with this. Just mark the outstanding comments as resolved and we can merge this.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13: Code-Review+2
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34535/2/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/2/src/lib/fit_payload.c@249 PS2, Line 249: initrd.size > 0
Please make that a separate check, so that you can print out a warning in this case. […]
Done
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/3/src/lib/fit_payload.c@249 PS3, Line 249: size
Ok, I added new if-statement to throw an error when the initrd size is 0 and updated the description […]
Done
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/5/src/lib/fit_payload.c@250 PS5, Line 250: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n");
Checking the size in extract() is better. I moved it to extract(). Thanks.
Done
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/7/src/lib/fit_payload.c@56 PS7, Line 56:
I update to return true when the size is 0 because I believe that an empty kernel/initrd is not our […]
Done
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@54 PS10, Line 54: region
I replaced with node. […]
Done
https://review.coreboot.org/c/coreboot/+/34535/10/src/lib/fit_payload.c@78 PS10, Line 78: memcpy(dst, node->data, node->size);
I think the situation "region->size < node->size" never happens in the current implementation. […]
Done
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/34535/12/src/lib/fit_payload.c@255 PS12, Line 255: printk(BIOS_ERR, "ERROR: The initrd size is 0.\n");
This was my mistake happened during rebasing. I deleted them. Thank you.
Done
Asami Doi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34535/2//COMMIT_MSG@11 PS2, Line 11: don't know extract
Sorry, this part doesn't make sense at all. I updated the description.
Done
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34535 )
Change subject: lib: Throw an error when ramdisk is present but initrd.size is 0 ......................................................................
lib: Throw an error when ramdisk is present but initrd.size is 0
It fails if you call extract() when ramdisk is present but initrd size is 0. This CL adds if-statement to throw an error when initrd size is 0.
Change-Id: I85aa33d2c2846b6b3a58df834dda18c47433257d Signed-off-by: Asami Doi d0iasm.pub@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34535 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Martin Roth martinroth@google.com --- M src/lib/fit_payload.c 1 file changed, 5 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Julius Werner: Looks good to me, approved Raul Rangel: Looks good to me, approved Asami Doi: Looks good to me, but someone else must approve
diff --git a/src/lib/fit_payload.c b/src/lib/fit_payload.c index a4d3705..1b6c986 100644 --- a/src/lib/fit_payload.c +++ b/src/lib/fit_payload.c @@ -51,6 +51,11 @@ const char *comp_name; size_t true_size = 0;
+ if (node->size == 0) { + printk(BIOS_ERR, "ERROR: The %s size is 0\n", node->name); + return true; + } + switch (node->compression) { case CBFS_COMPRESS_NONE: comp_name = "Relocating uncompressed";
-
Asami Doi (Code Review) -
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)