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

[Oliver Steffen]

On Tue, Feb 28, 2023 at 11:31 AM 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).

I think something like a suffix for %p is a good idea.  Like Linux kernel does:

https://www.kernel.org/doc/Documentation/core-api/printk-formats.rst

This avoids the warnings about the format string.  I am proposing %pG for GUIDs.

I am almost done implementing this, incl. the GUID unification, and also with the

comments from v2.

- Oliver