Re: [PATCH v3] Mandatory install device check for PowerPC
On Wed, Oct 15, 2025 at 02:22:18PM +0530, Avnish Chouhan wrote:
> On 2025-10-13 15:37, Michal Suchánek wrote:
> > On Mon, Oct 13, 2025 at 03:09:59PM +0530, Avnish Chouhan wrote:
> > > This patch adds a check on install_device while installing grub for
> > > PowerPC.
> > > If install_device is not mentioned in grub2-install and machine is
> > > detected
> > > as PowerPC, the error will be thrown and it will terminates the
> > > grub2-install
> > > operation. Running grub2-install on PowerPC without the
> > > install_device may
> > > result in bootlist corruption. When no install device is specified,
> > > it attempts
> > > to load images from the filesystem, which leads to nvram bootlist
> > > corruption.
> > > The idea is to fail the operation and avoid creating the invalid
> > > boot entry.
> >
>
> Hi Michal,
> Hope you're doing wonderful!
> Thank you so much for your response!
>
> Surely the changes can be done the way you mentioned. But in my honest
> opinion, keeping this check in "else" make the code looks lot more
> understandable (if we see the complete if else condition). Keeping this in
> as added "else" part clearly stats that this check is only specific to
> PowerPC. And it has no impact on PowerMac.
> Thank you!
As has been pointed out in the other subthread this is in fact
confusing.
The nesting in an else clause makes this code implicitly non-mac, while
reducing the nesting and explicitly checking is_prep makes it clearly
non-mac.
Thanks
Michal
>
> Regards,
> Avnish Chouhan
>
> > Hello,
> >
> > there is the part
> > if (install_device)
> > is_prep = 1;
> >
> > The check added by this patch now assumes PReP.
> >
> > For consistency this should be set unconditionally now.
> >
> >
> > > Signed-off-by: Avnish Chouhan
> > > ---
> > > util/grub-install.c | 13 +
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/util/grub-install.c b/util/grub-install.c
> > > index 0602465..f7389b3 100644
> > > --- a/util/grub-install.c
> > > +++ b/util/grub-install.c
> > > @@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
> > > is_prep = 0;
> > > }
> > > }
> > > +#if defined(__powerpc__)
> > > + else
> > > + {
> > > + /*
> > > +* As the machine has been detected as PowerPC and not the
> > > PowerMac. We need to check
> > > +* whether the install_device has been mentioned while
> > > installing. If no device has been
> > > +* mentioned, we need to exit and mark it as an error as the
> > > install_device is required for
> > > +* PowerPC installation. An installation with no device
> > > mentioned may lead to corruptions.
> > > +*/
> > > + if (!install_device)
> > Then instead of nesting inside else you could change the condition to
> >
> > if (is_prep && !install_device)
> >
> > That clarifies what it is checking for, and will cover the cases when
> > there is a partition that is checked for macppc content but none is
> > found.
> >
> > Thanks
> >
> > Michal
> > > +grub_util_error ("%s", _("install device isn't
> > > specified required for PowerPC"));
> >
> > I think this can be worded better.
> >
> > Something like "No install device specified and no PowerPC Mac partition
> > found."
> >
> > Thanks
> >
> > Michal
> >
> > > + }
> > > +#endif /* __powerpc__ */
> > > }
> > >
> > >size_t ndev = 0;
> > > --
> > > 2.47.3
> > >
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Mandatory install device check for PowerPC
On Mon, Oct 13, 2025 at 11:56:41AM +0200, John Paul Adrian Glaubitz wrote:
> Hello Avnish,
>
> On Mon, 2025-10-13 at 15:09 +0530, Avnish Chouhan wrote:
> > diff --git a/util/grub-install.c b/util/grub-install.c
> > index 0602465..f7389b3 100644
> > --- a/util/grub-install.c
> > +++ b/util/grub-install.c
> > @@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
> > is_prep = 0;
> > }
> > }
> > +#if defined(__powerpc__)
> > + else
> > + {
> > + /*
> > + * As the machine has been detected as PowerPC and not the PowerMac.
> > We need to check
>
> I have not looked at the patch context here, so I'm just asking: Where are
> you checking that the
> target machine is not a PowerMac where installation isn't performed on a
> device but into a directory?
>
> Should there be a check whether --macppc-directory is set?
It's in the else branch of that check. It could be written clearer,
though.
Thanks
Michal
> Also, minor nitpick: It should be just "As the machine has been detected as
> PowerPC and not a PowerMac".
>
> > + * whether the install_device has been mentioned while installing. If
> > no device has been
> > + * mentioned, we need to exit and mark it as an error as the
> > install_device is required for
> > + * PowerPC installation. An installation with no device mentioned may
> > lead to corruptions.
> > + */
> > + if (!install_device)
> > +grub_util_error ("%s", _("install device isn't specified
> > required for PowerPC"));
> > + }
> > +#endif /* __powerpc__ */
> > }
> >
> >size_t ndev = 0;
>
> Adrian
>
> --
> .''`. John Paul Adrian Glaubitz
> : :' : Debian Developer
> `. `' Physicist
> `-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Mandatory install device check for PowerPC
On 2025-10-13 15:37, Michal Suchánek wrote:
On Mon, Oct 13, 2025 at 03:09:59PM +0530, Avnish Chouhan wrote:
This patch adds a check on install_device while installing grub for
PowerPC.
If install_device is not mentioned in grub2-install and machine is
detected
as PowerPC, the error will be thrown and it will terminates the
grub2-install
operation. Running grub2-install on PowerPC without the install_device
may
result in bootlist corruption. When no install device is specified, it
attempts
to load images from the filesystem, which leads to nvram bootlist
corruption.
The idea is to fail the operation and avoid creating the invalid boot
entry.
Hi Michal,
Hope you're doing wonderful!
Thank you so much for your response!
Surely the changes can be done the way you mentioned. But in my honest
opinion, keeping this check in "else" make the code looks lot more
understandable (if we see the complete if else condition). Keeping this
in as added "else" part clearly stats that this check is only specific
to PowerPC. And it has no impact on PowerMac.
Thank you!
Regards,
Avnish Chouhan
Hello,
there is the part
if (install_device)
is_prep = 1;
The check added by this patch now assumes PReP.
For consistency this should be set unconditionally now.
Signed-off-by: Avnish Chouhan
---
util/grub-install.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/util/grub-install.c b/util/grub-install.c
index 0602465..f7389b3 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
is_prep = 0;
}
}
+#if defined(__powerpc__)
+ else
+ {
+ /*
+ * As the machine has been detected as PowerPC and not the
PowerMac. We need to check
+ * whether the install_device has been mentioned while installing.
If no device has been
+ * mentioned, we need to exit and mark it as an error as the
install_device is required for
+ * PowerPC installation. An installation with no device mentioned
may lead to corruptions.
+ */
+ if (!install_device)
Then instead of nesting inside else you could change the condition to
if (is_prep && !install_device)
That clarifies what it is checking for, and will cover the cases when
there is a partition that is checked for macppc content but none is
found.
Thanks
Michal
+grub_util_error ("%s", _("install device isn't specified
required for PowerPC"));
I think this can be worded better.
Something like "No install device specified and no PowerPC Mac
partition found."
Thanks
Michal
+ }
+#endif /* __powerpc__ */
}
size_t ndev = 0;
--
2.47.3
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Mandatory install device check for PowerPC
On 2025-10-13 15:26, John Paul Adrian Glaubitz wrote:
Hello Avnish,
On Mon, 2025-10-13 at 15:09 +0530, Avnish Chouhan wrote:
diff --git a/util/grub-install.c b/util/grub-install.c
index 0602465..f7389b3 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
is_prep = 0;
}
}
+#if defined(__powerpc__)
+ else
+ {
+ /*
+ * As the machine has been detected as PowerPC and not the
PowerMac. We need to check
I have not looked at the patch context here, so I'm just asking: Where
are you checking that the
target machine is not a PowerMac where installation isn't performed on
a device but into a directory?
Should there be a check whether --macppc-directory is set?
Hi Adrian,
The code block just above this "if (macppcdir).." checks whether the
machine is a "PowerMac" or not a "PowerMac". We have just added the else
part of "if (macppcdir)" !
Thank you!
Regards,
Avnish Chouhan
Also, minor nitpick: It should be just "As the machine has been
detected as PowerPC and not a PowerMac".
+ * whether the install_device has been mentioned while installing.
If no device has been
+ * mentioned, we need to exit and mark it as an error as the
install_device is required for
+ * PowerPC installation. An installation with no device mentioned
may lead to corruptions.
+ */
+ if (!install_device)
+grub_util_error ("%s", _("install device isn't specified
required for PowerPC"));
+ }
+#endif /* __powerpc__ */
}
size_t ndev = 0;
Adrian
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Mandatory install device check for PowerPC
Hello Avnish,
On Mon, 2025-10-13 at 15:09 +0530, Avnish Chouhan wrote:
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 0602465..f7389b3 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
> is_prep = 0;
> }
> }
> +#if defined(__powerpc__)
> + else
> + {
> + /*
> +* As the machine has been detected as PowerPC and not the PowerMac.
> We need to check
I have not looked at the patch context here, so I'm just asking: Where are you
checking that the
target machine is not a PowerMac where installation isn't performed on a device
but into a directory?
Should there be a check whether --macppc-directory is set?
Also, minor nitpick: It should be just "As the machine has been detected as
PowerPC and not a PowerMac".
> +* whether the install_device has been mentioned while installing. If
> no device has been
> +* mentioned, we need to exit and mark it as an error as the
> install_device is required for
> +* PowerPC installation. An installation with no device mentioned may
> lead to corruptions.
> +*/
> + if (!install_device)
> +grub_util_error ("%s", _("install device isn't specified
> required for PowerPC"));
> + }
> +#endif /* __powerpc__ */
> }
>
>size_t ndev = 0;
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer
`. `' Physicist
`-GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v3] Mandatory install device check for PowerPC
On Mon, Oct 13, 2025 at 03:09:59PM +0530, Avnish Chouhan wrote:
> This patch adds a check on install_device while installing grub for PowerPC.
> If install_device is not mentioned in grub2-install and machine is detected
> as PowerPC, the error will be thrown and it will terminates the grub2-install
> operation. Running grub2-install on PowerPC without the install_device may
> result in bootlist corruption. When no install device is specified, it
> attempts
> to load images from the filesystem, which leads to nvram bootlist corruption.
> The idea is to fail the operation and avoid creating the invalid boot entry.
Hello,
there is the part
if (install_device)
is_prep = 1;
The check added by this patch now assumes PReP.
For consistency this should be set unconditionally now.
> Signed-off-by: Avnish Chouhan
> ---
> util/grub-install.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 0602465..f7389b3 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -1289,6 +1289,19 @@ main (int argc, char *argv[])
> is_prep = 0;
> }
> }
> +#if defined(__powerpc__)
> + else
> + {
> + /*
> +* As the machine has been detected as PowerPC and not the PowerMac.
> We need to check
> +* whether the install_device has been mentioned while installing. If
> no device has been
> +* mentioned, we need to exit and mark it as an error as the
> install_device is required for
> +* PowerPC installation. An installation with no device mentioned may
> lead to corruptions.
> +*/
> + if (!install_device)
Then instead of nesting inside else you could change the condition to
if (is_prep && !install_device)
That clarifies what it is checking for, and will cover the cases when
there is a partition that is checked for macppc content but none is
found.
Thanks
Michal
> +grub_util_error ("%s", _("install device isn't specified
> required for PowerPC"));
I think this can be worded better.
Something like "No install device specified and no PowerPC Mac partition found."
Thanks
Michal
> + }
> +#endif /* __powerpc__ */
> }
>
>size_t ndev = 0;
> --
> 2.47.3
>
___
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
