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? > > > + if (!*part_uuid) > > > + { > > > + status = grub_errno; > > > + } > > > > Please drop redundant braces. > > > > > +finish: > > > > Labels should be prefixed with a space... > > > > > + grub_disk_close (disk); > > > + > > > + return status; > > > +} > > > + > > > +static grub_err_t > > > +set_efi_str_variable (const char *name, const grub_efi_guid_t *guid, > > > + const char *value) > > > +{ > > > + grub_size_t len; > > > + grub_size_t len16; > > > > grub_size_t len, len16; > > > > > + grub_efi_char16_t *value_16; > > > + grub_err_t status; > > > + > > > + len = grub_strlen (value); > > > + len16 = len * GRUB_MAX_UTF16_PER_UTF8; > > > > What will happen when len == 0? > > > > Is there any chance for overflow here? If yes please use overflow aware > > operators from include/grub/safemath.h here. > > Fixing this. Same problem also in grub_efi_set_variable_with_attributes. > This is were I found this code. Fill fix there too. Cool! Thanks a lot! > Thanks for the review. I will send out v2 shortly. Great! Thanks, Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
