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

[Christian Marangi]

On Mon, Jan 09, 2023 at 02:34:48PM -0800, Brian Norris wrote:
> 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.
>

This is what gets me most confused... The patch is correct and looks
good... Just what I can't understand is how things worked till today...

They all fallback to rootdisk.c? It's worth to check and have some proof
of this theory.

Since we are playing with the function mounting root the main concern
here is that we brake any device using PARTUUID that somehow are working
now so we need to be carefull in what we change as the risk of causing
regression is behind the corner.

So we should just find a way to understand why thing are working (or are
not working and are using an alternative way currently) Just that and
this can be merged.

> > 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.)
> 

I understand and I want this fixed. Sorry if I look confused but you can
understand how this was broken from ages and still we have many target
using the root=PARTUUID format makes me question a lot of thing ahahhaha

> 
> > > ---
> > >
> > > 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

-- 
	Ansuel