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 <xxc3ncore...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
