Hey :)
On Wed, 2025-11-26 at 11:25 +0100, Ahmad Fatoum wrote:
>
> On 11/26/25 7:42 AM, Fabian Pflug wrote:
> > Getting root device adn root kernel options seperatily any combining
>
> device and root kernel options seperatly and combining
>
> > them in this function instead of getting the full string from the helper
> > functions. This is in preperation for replacing the root= string in the
>
> preparation
>
> > kernel commandline with something defined during runtime.
> >
> > Signed-off-by: Fabian Pflug <[email protected]>
> > ---
> > common/bootm.c | 34 ++++++++++++++++++++--------------
> > 1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 17792b2a1d..5d7dc00e3e 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -4,6 +4,7 @@
> > #include <bootm.h>
> > #include <bootm-overrides.h>
> > #include <fs.h>
> > +#include <fcntl.h>
> > #include <malloc.h>
> > #include <memory.h>
> > #include <block.h>
> > @@ -841,33 +842,38 @@ int bootm_boot(struct bootm_data *bootm_data)
> > }
> >
> > if (bootm_data->appendroot) {
> > - char *rootarg;
> > + char *root;
> > + char *rootopts;
> >
> > if (bootm_data->root_dev) {
> > const char *root_dev_name =
> > devpath_to_name(bootm_data->root_dev);
> > struct cdev *root_cdev =
> > cdev_open_by_name(root_dev_name, O_RDONLY);
> >
> > - rootarg = cdev_get_linux_rootarg(root_cdev);
> > - if (!rootarg) {
> > - rootarg = ERR_PTR(-EINVAL);
> > -
> > - if (!root_cdev)
> > - pr_err("no cdev found for %s, cannot
> > set root= option\n",
> > - root_dev_name);
> > - else if (!root_cdev->partuuid[0])
> > - pr_err("%s doesn't have a PARTUUID,
> > cannot set root= option\n",
> > - root_dev_name);
>
> This warning is useful, because it's very confusing otherwise when a MBR
> disk image is created without a NT signature.
>
> > - }
> > + cdev_get_linux_root_and_opts(root_cdev, &root,
> > &rootopts);
> >
> > if (root_cdev)
> > cdev_close(root_cdev);
> > + else
> > + pr_err("no cdev found for %s, cannot set root=
> > option\n", root_dev_name);
> > } else {
> > - rootarg = path_get_linux_rootarg(data->os_file);
>
> path_get_linux_rootarg() is stale now right? Can you squash its removal
> into the patch that gets rid of linux.bootargs?
Then we would have a case, where the function is used, but not available
anymore resulting in an unbuildable system,
which I would like to avoid. I can remove it in an extra commit after this one.
>
> > + struct fs_device *fsdev =
> > get_fsdevice_by_path(AT_FDCWD, data->os_file);
> > + if(fsdev)
> > + fsdev_get_linux_root_options(fsdev, &root,
> > &rootopts);
> > + else
> > + pr_err("no fsdevice under path: %s\n",
> > data->os_file);
> > }
> >
> > - if (IS_ERR(rootarg)) {
> > + if (!root) {
> > pr_err("Failed to append kernel cmdline parameter
> > 'root='\n");
> > } else {
> > + char *rootarg;
> > + if(rootopts) {
> > + rootarg = xasprintf("root=%s %s", root,
> > rootopts);
> > + free(rootopts);
> > + } else {
> > + rootarg = xasprintf("root=%s", root);
> > + }
>
> I wonder if we should add a helper into a header that does this, given
> how many times this is repeated...
>
> char *format_root_bootarg(char *root, char *rootopts);
>
> which consumes (and frees) two pointers and returns a new buffer..
Good thinking. But where do we add it?
* fs.h
* block.h
* bootargs.h
And also, do we add a third parameter? because I will re replaced in the next
commit with "%s=%s %s"
And since you proposed to get rid of path_get_linux_rootarg it will be used
with two parameters in one place
(cdev_get_linux_rootarg) and with three at the other. (bootm_boot)
Kind regard
Fabian
>
> Cheers,
> Ahmad
>
> > + free(root);
> > pr_info("Adding \"%s\" to Kernel commandline\n",
> > rootarg);
> > globalvar_add_simple("linux.bootargs.bootm.appendroot",
> > rootarg);
