On Tue, Feb 28, 2023 at 11:31 AM Oliver Steffen <ostef...@redhat.com> wrote:
> On Tue, Feb 21, 2023 at 4:39 PM Daniel Kiper <dki...@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 <ostef...@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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
