[PATCH fstools v2] partname: Ignore root=PARTUUID...

[Brian Norris]

On Mon, Jan 9, 2023 at 1:53 PM Christian Marangi <ansuelsmth at gmail.com> wrote:
>
> On Fri, Jan 06, 2023 at 06:04:22PM -0800, Brian Norris wrote:
> > We're assuming all root= arguments are /dev/ paths, but many targets
> > utilize root=PARTUUID=<xxx> strategies. At least allow them to fall back
> > to scanning all block devices.
> >
> > Signed-off-by: Brian Norris <computersforpeace at gmail.com>
>
> Can you elaborate this a bit more?

This function currently assumes that if it can find a "root=" line,
then it can parse it and use it to find "rootfs_data" on the same
disk. But the rootdevname() function really chokes (does completely
the wrong thing) when given "PARTUUID=<blah>". So this parser becomes
useless.

> This is dependent of the google based
> devices with ipq806x but why?

I guess it's not *strictly* a dependency, but as-is, things aren't great.

With the above choking, I believe fstools will fall back to
rootdisk.c, which will place rootfs_data in a different place --
appended to the squashfs filesystem, via a custom loopback device.
This works OK I suppose, but has its downsides.

But the real kicker is that if fstools / partname.c eventually learns
how to find the right 'rootfs_data' partition, then suddenly our data
filesystem will move, and that's not great for sysupgrade. So I'd like
to get it right from the start, rather than change between rootdisk.c
and partname.c approaches arbitrarily.

> In theory we should have other device with sd card/eMMC that use uuid to
> select the partition...

Right. Search the tree for the "root=PARTUUID=" string, and you'll
find several. (Some breadcrumbs in rockchip, x86, imx, and more.)
Presumably they would run into the same problem if they tried to use a
dedicated data partition on a block device -- right now, I believe
Rockchip restricts itself to a 2-partition layout, for instance. Which
is why I didn't specifically call this a ipq806x / Google-specific
thing.

> Also can't we just ignore the device bootargs
> and provide a custom one?

This isn't about the device (as in, baked into a bootloader) bootargs;
see "root=PARTUUID=%U/PARTNROFF=1" in my patch 7:
https://patchwork.ozlabs.org/project/openwrt/patch/20230107074945.2140362-7-computersforpeace@gmail.com/
This is a better way of specifying root devices (say, rather than
memorizing a "/dev/mmcblk0" or similar, which is *not* a stable
contract; it also means boot-from-USB won't work), except that fstools
doesn't like it. (And again, there are several other existing
openwrt.git users for it.)

Brian

> > ---
> >
> > Changes in v2:
> >  * fstools, not fsutils (sorry for the noise)
> >
> >  libfstools/partname.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libfstools/partname.c b/libfstools/partname.c
> > index f59c52eb8f3c..9c27015643ad 100644
> > --- a/libfstools/partname.c
> > +++ b/libfstools/partname.c
> > @@ -128,12 +128,12 @@ static struct volume *partname_volume_find(char *name)
> >                       return NULL;
> >       }
> >
> > -     if (get_var_from_file("/proc/cmdline", "root", rootparam, sizeof(rootparam))) {
> > +     if (get_var_from_file("/proc/cmdline", "root", rootparam, sizeof(rootparam)) && rootparam[0] == '/') {
> >               rootdev = rootdevname(rootparam);
> >               /* find partition on same device as rootfs */
> >               snprintf(ueventgstr, sizeof(ueventgstr), "%s/%s/*/uevent", block_dir_name, rootdev);
> >       } else {
> > -             /* no 'root=' kernel cmdline parameter, find on any block device */
> > +             /* no useful 'root=' kernel cmdline parameter, find on any block device */
> >               snprintf(ueventgstr, sizeof(ueventgstr), "%s/*/uevent", block_dir_name);
> >       }
> >
> > --
> > 2.39.0
> >
> >
> > _______________________________________________
> > openwrt-devel mailing list
> > openwrt-devel at lists.openwrt.org
> > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
>
> --
>         Ansuel