-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
u-boot: add 'bootdir' to the generated uEnv.txt #3271
base: main
Are you sure you want to change the base?
Conversation
Hi @ChenQi1989. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
@martinezjavier I think you wrote this code, could we borrow your eyes for a quick review? |
@@ -106,6 +106,7 @@ create_config_from_boot_loader_entries (OstreeBootloaderUboot *self, int bootver | |||
g_autoptr (GPtrArray) boot_loader_configs = NULL; | |||
OstreeBootconfigParser *config; | |||
const char *val; | |||
g_autofree char *bootdir = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better if we changed the scope of this variable into the for loop, like the "index_suffix" variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericcurtin Thanks. I've refreshed this PR.
When doing a full copy of: $deployment/usr/lib/ostree-boot -> /boot/ostree/$os-$bootcsum/ U-Boot bootscript can use the 'bootdir' to find, for example, the Device Tree (dtb) file, as in: load ${dtype} ${disk}:${bootpart} ${a_fdt} ${bootdir}${dtbname} Or u-boot external bootscript: load ${dtype} ${disk}:${bootpart} ${a_scr} ${bootdir}${scriptname} It could also be possible to point 'bootdir' directly to the $deployment/usr/lib/ostree-boot, but this would add unnecessary restrictions on what file system can be used for rootfs as u-boot, for example, can not read from BTRFS. So having bootdir=/boot/ostree/$os-$bootcsum/ is a better approach here, as /boot can be on a separate partition with its own file system type. Signed-off-by: Gatis Paeglis <[email protected]> Signed-off-by: Hongxu Jia <[email protected]> Signed-off-by: Chen Qi <[email protected]>
The patch makes sense to me. I wonder if |
@@ -129,6 +130,9 @@ create_config_from_boot_loader_entries (OstreeBootloaderUboot *self, int bootver | |||
} | |||
g_ptr_array_add (new_lines, g_strdup_printf ("kernel_image%s=/boot%s", index_suffix, val)); | |||
|
|||
bootdir = strndup (val, strrchr(val, '/') - val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this effectively dirname()
? We do this across the codebase:
g_autofree char *tmpbuf = g_strdup (dir_or_file_path);
const char *dir = dirname (tmpbuf);
When doing a full copy of:
$deployment/usr/lib/ostree-boot -> /boot/ostree/$os-$bootcsum/
U-Boot bootscript can use the 'bootdir' to find, for example, the Device Tree (dtb) file, as in:
load ${dtype} ${disk}:${bootpart} ${a_fdt} ${bootdir}${dtbname}
Or u-boot external bootscript:
load ${dtype} ${disk}:${bootpart} ${a_scr} ${bootdir}${scriptname}
It could also be possible to point 'bootdir' directly to the $deployment/usr/lib/ostree-boot, but this would add unnecessary restrictions on what file system can be used for rootfs as u-boot, for example, can not read from BTRFS. So having
bootdir=/boot/ostree/$os-$bootcsum/ is a better approach here, as /boot can be on a separate partition with its own file system type.