Re: [PATCH v2] util/grub.d/linux: Improve initramfs detection

[Daniel Kiper]

Sorry, somehow I missed your reply...

On Thu, Apr 07, 2022 at 09:35:19PM -0500, Oskari Pirhonen wrote:
> On Thu, Apr 07, 2022 at 05:44:18PM +0200, Daniel Kiper wrote:
> > On Sun, Mar 27, 2022 at 10:41:31PM -0500, Oskari Pirhonen wrote:
> > > Add detection for initramfs of the form *.img.old. For example, Gentoo's
> > > sys-kernel/genkernel installs it as initramfs-*.img and moves any
> > > existing one to initramfs-*.img.old.
> >
> > You are mentioning initramfs* files but the patch adds also initrd*
> > files. Could you explain that in the commit message?
>
> Yeah, I can do that.

Cool! Thanks!

> > > Signed-off-by: Oskari Pirhonen <xxc3ncoredxx@gmail.com>
> >
> > You are breaking indention in the patch.
> >
> > > ---
> > > v1 -> v2:
> > > - don't reorder the checks
> > > - include 20_linux_xen.in
> > >
> > >  util/grub.d/10_linux.in     | 17 +++++++++--------
> > >  util/grub.d/20_linux_xen.in | 29 +++++++++++++++--------------
> > >  2 files changed, 24 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> > > index ca068038e..cb049e943 100644
> > > --- a/util/grub.d/10_linux.in
> > > +++ b/util/grub.d/10_linux.in
> > > @@ -215,14 +215,15 @@ while [ "x$list" != "x" ] ; do
> > >    done
> > >
> > >    initrd_real=
> > > -  for i in "initrd.img-${version}" "initrd-${version}.img" 
> > > "initrd-${version}.gz" \
> > > -    "initrd-${version}" "initramfs-${version}.img" \
> > > -    "initrd.img-${alt_version}" "initrd-${alt_version}.img" \
> > > -    "initrd-${alt_version}" "initramfs-${alt_version}.img" \
> > > -    "initramfs-genkernel-${version}" \
> > > -    "initramfs-genkernel-${alt_version}" \
> > > -    "initramfs-genkernel-${GENKERNEL_ARCH}-${version}" \
> > > -    "initramfs-genkernel-${GENKERNEL_ARCH}-${alt_version}"; do
> > > +  for i in "initrd.img-${version}" "initrd-${version}.img" \
> > > +        "initrd-${alt_version}.img.old" "initrd-${version}.gz" \
> > > +        "initrd-${alt_version}.gz.old" "initrd-${version}" \
> > > +        "initramfs-${version}.img" "initramfs-${alt_version}.img.old" \
> > > +        "initrd.img-${alt_version}" "initrd-${alt_version}.img" \
> > > +        "initrd-${alt_version}" "initramfs-${alt_version}.img" \
> > > +        "initramfs-genkernel-${version}" 
> > > "initramfs-genkernel-${alt_version}" \
> > > +        "initramfs-genkernel-${GENKERNEL_ARCH}-${version}" \
> >
> > Here...
> >
> > > +        "initramfs-genkernel-${GENKERNEL_ARCH}-${alt_version}"; do
> >
> > Ditto...
> >
> > >      if test -e "${dirname}/${i}" ; then
> > >        initrd_real="${i}"
> > >        break
> > > diff --git a/util/grub.d/20_linux_xen.in b/util/grub.d/20_linux_xen.in
> > > index f45559ff8..5a1b7b7d4 100644
> > > --- a/util/grub.d/20_linux_xen.in
> > > +++ b/util/grub.d/20_linux_xen.in
> > > @@ -283,20 +283,21 @@ while [ "x${xen_list}" != "x" ] ; do
> > >   alt_version=`echo $version | sed -e "s,\.old$,,g"`
> > >   linux_root_device_thisversion="${LINUX_ROOT_DEVICE}"
> > >
> > > - initrd_real=
> > > - for i in "initrd.img-${version}" "initrd-${version}.img" 
> > > "initrd-${version}.gz" \
> > > -    "initrd-${version}" "initramfs-${version}.img" \
> > > -    "initrd.img-${alt_version}" "initrd-${alt_version}.img" \
> > > -    "initrd-${alt_version}" "initramfs-${alt_version}.img" \
> > > -    "initramfs-genkernel-${version}" \
> > > -    "initramfs-genkernel-${alt_version}" \
> > > -    "initramfs-genkernel-${GENKERNEL_ARCH}-${version}" \
> > > -    "initramfs-genkernel-${GENKERNEL_ARCH}-${alt_version}" ; do
> > > -     if test -e "${dirname}/${i}" ; then
> > > -         initrd_real="$i"
> > > -         break
> > > -     fi
> > > - done
> > > +    initrd_real=
> >
> > And here...
> >
> > > +    for i in "initrd.img-${version}" "initrd-${version}.img" \
> > > +        "initrd-${alt_version}.img.old" "initrd-${version}.gz" \
> > > +        "initrd-${alt_version}.gz.old" "initrd-${version}" \
> > > +        "initramfs-${version}.img" "initramfs-${alt_version}.img.old" \
> > > +        "initrd.img-${alt_version}" "initrd-${alt_version}.img" \
> > > +        "initrd-${alt_version}" "initramfs-${alt_version}.img" \
> > > +        "initramfs-genkernel-${version}" 
> > > "initramfs-genkernel-${alt_version}" \
> >
> > I would leave these two file names in separate lines. Same above...
>
> OK.
>
> >
> > > +        "initramfs-genkernel-${GENKERNEL_ARCH}-${version}" \
> > > +        "initramfs-genkernel-${GENKERNEL_ARCH}-${alt_version}"; do
> > > +        if test -e "${dirname}/${i}" ; then
> > > +            initrd_real="${i}"
> > > +            break
> > > +        fi
> > > +    done
> >
> > Again, broken indention... It is almost in every line of this patch.
>
> That is actually something I wanted to address in a separate thread. The
> indentation/style feels kind of all over the place in the few files in
> grub.d/ that I glanced at. Some lines are indented with spaces, others
> with tabs. Sometimes variables are substituted with $var, other times
> with ${var}. Sometimes if [ ... ] is used, other times if test ... is.

Try to keep with a style in a given file in util/grub.d.

> If there isn't already an existing style guide that I can reference, I
> would like to establish one and then re-style these scripts to be
> consistent with that. But it'd have to be a separate endeavor in that
> case.

Yeah, I think it would be nice to fix that mess at some point.

> In the meantime, how would you like me to indent the lines in this
> patch?

If you need more than 7 spaces then use tabs first and spaces after that
as needed.

Daniel