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

[Oskari Pirhonen <xxc3ncoredxx@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5320 bytes --]

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.

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

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.

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

- Oskari

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]