On Tue, 28 Feb 2023 11:31:41 +0100 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). > > 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
