Re: [PATCH v1 2/2] Add a module for the Boot Loader Interface

[Glenn Washburn]

On Tue, 28 Feb 2023 11:31:41 +0100
Oliver Steffen <osteffen@redhat.com> wrote:

> On Tue, Feb 21, 2023 at 4:39 PM Daniel Kiper <dkiper@net-space.pl>
> wrote:
> 
> > On Mon, Feb 20, 2023 at 08:15:35AM -0800, Oliver Steffen wrote:
> > > Thank you for the comments, Daniel.
> > >
> > > Quoting Daniel Kiper (2023-02-15 19:27:03)
> > > > On Mon, Jan 16, 2023 at 12:40:53PM +0100, Oliver Steffen wrote:
> > > > > Add a new module named boot_loader_interface, which provides a
> > command
> > > >
> > > > I would prefer something shorter than boot_loader_interface.
> > > > bli?
> > >
> > > Then bli it is.
> >
> > Cool! Thanks!
> >
> > > > > with the same name. It implements a small but quite useful
> > > > > part of
> > the
> > > > > Boot Loader Interface [0].  This interface uses EFI variables
> > > > > for communication between the boot loader and the operating
> > > > > system.
> > > > >
> > > > > This module sets two EFI variables under the vendor GUID
> > > > > 4a67b082-0a4c-41cf-b6c7-440b29bb8c4f:
> > > > >
> > > > > - LoaderInfo: contains GRUB + <version number>.
> > > > >   This allows the running operating system to identify the
> > > > > boot
> > loader
> > > > >   used during boot.
> > > > >
> > > > > - LoaderDevicePartUUID: contains the partition UUID of the
> > > > >   EFI System Partition (ESP).  This is used by
> > > > >   systemd-gpt-auto-generator [1] to find the root partitions
> > > > > (and
> > others
> > > > >   too), via partition type IDs [2].
> > > > >
> > > > > This module is only available on EFI platforms.
> > > > >
> > > > > [0] https://systemd.io/BOOT_LOADER_INTERFACE/
> > > > > [1]
> > https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html
> > > > > [2]
> > https://uapi-group.org/specifications/specs/discoverable_partitions_specification/
> > > > >
> > > > > Signed-off-by: Oliver Steffen <osteffen@redhat.com>
> > > > > ---
> > > > >  grub-core/Makefile.core.def                |   6 +
> > > > >  grub-core/commands/boot_loader_interface.c | 217
> > +++++++++++++++++++++
> > > > >  2 files changed, 223 insertions(+)
> > > > >  create mode 100644 grub-core/commands/boot_loader_interface.c
> > > > >
> > > > > diff --git a/grub-core/Makefile.core.def
> > b/grub-core/Makefile.core.def
> > > > > index ba967aac8..23455fb71 100644
> > > > > --- a/grub-core/Makefile.core.def
> > > > > +++ b/grub-core/Makefile.core.def
> > > > > @@ -2547,3 +2547,9 @@ module = {
> > > > >    common = commands/i386/wrmsr.c;
> > > > >    enable = x86;
> > > > >  };
> > > > > +
> > > > > +module = {
> > > > > +  name = boot_loader_interface;
> > > >
> > > > s/boot_loader_interface/bli/?
> > > >
> > > > > +  efi = commands/boot_loader_interface.c;
> > > >
> > > > Ditto and below if needed...
> > > >
> > > > > +  enable = efi;
> > > > > +};
> > > > > diff --git a/grub-core/commands/boot_loader_interface.c
> > b/grub-core/commands/boot_loader_interface.c
> > > > > new file mode 100644
> > > > > index 000000000..ccd7fa3d9
> > > > > --- /dev/null
> > > > > +++ b/grub-core/commands/boot_loader_interface.c
> > > > > @@ -0,0 +1,217 @@
> > > > > +/*-*- Mode: C; c-basic-offset: 2; indent-tabs-mode: t -*-*/
> > > >
> > > > Could you move this to the end of the file? Good example you
> > > > can find
> > in
> > > > the Xen project [1].
> > >
> > > I personally do not care about this, it was just part of the file
> > > I used as a template. Is this wanted, or can we just drop it?
> >
> > If you do not need it please drop it.
> >
> > [...]
> >
> > > > > +  guid = &entry.guid;
> > > > > +  *part_uuid = grub_xasprintf (
> > > > > +      "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> > > > > +      grub_le_to_cpu32 (guid->data1), grub_le_to_cpu16
> > (guid->data2),
> > > > > +      grub_le_to_cpu16 (guid->data3), guid->data4[0],
> > guid->data4[1],
> > > > > +      guid->data4[2], guid->data4[3], guid->data4[4],
> > guid->data4[5],
> > > > > +      guid->data4[6], guid->data4[7]);
> > > >
> > > > I think this is generic thing and can be converted into a
> > > > function. Similar code is in the grub-core/commands/probe.c and
> > util/grub-probe.c.
> > > > It seems to me it is at least easy to make one function for the
> > > > GRUB core code. Please do that. Of course in separate patch.
> > >
> > > There are multiple representations of a GUID:
> > >  - grub_gpt_part_guid (gpt_partition.h)
> > >  - grub_efi_guid (efi/api.h)
> > >  - grub_efi_packed_guid (efi/api.h)
> > >
> > > I would add a function for the first kind to gpt_partition.h and
> > > partmap/gpt.c that prints into a string buffer (length checked).
> > >
> > > It would be nice to add a format specifier for GUIDs to the printf
> > > implementation,
> > > but that only makes much sense (I think) if the GUID
> > > repesentations could be unified into one first, in a central
> > > place.
> >
> > Could you do the unification and introduce format specifier for
> > GUIDs?
> >
> 
> Assume there is a common GUID struct.
> Then we have the following options (AFAIK):
> - add a format specifier for it, lets say '%g' or '%y'.
>   This causes problems with the -Wformat checks. We could
>   disable it, but that's a bad idea.
> - use an additional qualifier for %p, for example %p{GUID}.
>   This way gcc will not complain (%p takes void *). But it is
>   ugly if one wants to print a '{' after a GUID.
> - Do not use a custom format specifier at all; just provide a
>   "to-string" function. To print a guid one needs a temporary
>    buffer (less convenient, but still better than repeating the
>   raw printf statement for GUID everywhere, like it is now).
> 
> Pick your poison... :-)
> Or does anybody have a better idea?

What about doing similarly to what the kernel[1] does and have a %pG?
This is similar to your second option without the brackets and
accompanying issue. One issue with these both is that the argument
must be cast to a void*, which might be a little confusing to the
reader.

Here's the GCC issue[2] on adding extra format specifiers for the printf
checking. Its 10 years old and apparently no progress, so doesn't seem
like we'll get any help there.

Glenn

[1] https://www.kernel.org/doc/Documentation/printk-formats.txt
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=47781