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. > > > 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? > > > +/* boot_loader_interface.c - implementation of the boot loader interface > > + */ > > Please use correct formatting for comments [2] and move this one behind > the license below. > > > +/* > > + * GRUB -- GRand Unified Bootloader > > + * > > + * GRUB is free software: you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation, either version 3 of the License, or > > + * (at your option) any later version. > > + * > > + * GRUB is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with GRUB. If not, see <http://www.gnu.org/licenses/>. > > + */ > > Please take license from the grub-core/kern/efi/sb.c file and update > year in it. You can also merge module description with the license like > it is done in the grub-core/kern/efi/sb.c. > > > +#include <grub/charset.h> > > +#include <grub/efi/api.h> > > +#include <grub/efi/disk.h> > > +#include <grub/efi/efi.h> > > +#include <grub/err.h> > > +#include <grub/extcmd.h> > > +#include <grub/gpt_partition.h> > > +#include <grub/misc.h> > > +#include <grub/mm.h> > > +#include <grub/partition.h> > > + > > +GRUB_MOD_LICENSE ("GPLv3+"); > > + > > +#define MODNAME "boot_loader_interface" > > + > > +static const grub_efi_guid_t boot_loader_interface_vendor_guid = > > + { 0x4a67b082, 0x0a4c, 0x41cf, > > + {0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f} }; > > Please define this GUID as constant in the include/grub/efi/api.h (you > will find good examples how it should be done there). I expect it can > be useful for others too. > > > +static char * > > +machine_get_bootdevice (void) > > +{ > > + grub_efi_loaded_image_t *image; > > + > > + image = grub_efi_get_loaded_image (grub_efi_image_handle); > > + if (!image) > > Please compare pointers explicitly with NULL, e.g. "if (image == NULL)", > here and below... > > > + return NULL; > > + > > + return grub_efidisk_get_device_name (image->device_handle); > > +} > > + > > +static grub_err_t > > +get_part_uuid (grub_device_t dev, char **part_uuid) > > +{ > > + grub_err_t status = GRUB_ERR_NONE; > > + grub_disk_t disk; > > + struct grub_gpt_partentry entry; > > + grub_gpt_part_guid_t *guid; > > + > > + if (!dev || !dev->disk || !dev->disk->partition) > > + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid device")); > > + > > + disk = grub_disk_open (dev->disk->name); > > + if (!disk) > > + { > > + status = grub_errno; > > + grub_dprintf (MODNAME, "Error opening disk\n"); > > + return grub_errno; > > s/return grub_errno/return status/? > > > + } > > + > > + if (grub_strcmp (dev->disk->partition->partmap->name, "gpt") != 0) > > + { > > + status = grub_error (GRUB_ERR_BAD_PART_TABLE, > > + N_("This is not a GPT partition table")); > > All errors in the GRUB start with lower case, i.e. s/This is/this is/. > > > + goto finish; > > s/finish/fail/ and below please... > > > + } > > + > > + if (grub_disk_read (disk, dev->disk->partition->offset, > > + dev->disk->partition->index, sizeof (entry), &entry)) > > ..., , &entry) != GRUB_ERR_NONE) > > > + { > > + status = grub_errno; > > + grub_dprintf (MODNAME, "%s: Read error\n", dev->disk->name); > > + goto finish; > > + } > > + > > + 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. > > + 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. > > > + value_16 = grub_calloc (len16 + 1, sizeof (value_16[0])); > > + if (!value_16) > > + return grub_errno; > > + > > + len16 > > + = grub_utf8_to_utf16 (value_16, len16, (grub_uint8_t *)value, len, > > NULL); > > Please do not break lines in that way. It is difficult to read. > > Here you can just do > > len16 = grub_utf8_to_utf16 (value_16, len16, (grub_uint8_t *)value, len, > NULL); > > > + value_16[len16] = 0; > > + > > + status = grub_efi_set_variable_with_attributes ( > > + name, guid, (void *)value_16, (len16 + 1) * sizeof (value_16[0]), > > s/(void *)value_16/(void *) value_16/ > > Casts require a space... Please fix this kinds of problems everywhere. > > > + GRUB_EFI_VARIABLE_BOOTSERVICE_ACCESS | > > GRUB_EFI_VARIABLE_RUNTIME_ACCESS); > > + if (status != GRUB_ERR_NONE) > > Yeah, I like this explicit comparison... > > > + { > > + grub_dprintf (MODNAME, "Error setting EFI variable %s: %d\n", name, > > + status); > > + } > > Please do not add braces if they are not needed... > > > + grub_free (value_16); > > + > > + return status; > > +} > > + > > +static grub_err_t > > +set_loader_info (void) > > +{ > > + grub_err_t status; > > You do not need the status variable here. Just return immediately status from > the > set_efi_str_variable() call. > > > + status = set_efi_str_variable ( > > + "LoaderInfo", &boot_loader_interface_vendor_guid, PACKAGE_STRING); > > return set_efi_str_variable ("LoaderInfo", > &boot_loader_interface_vendor_guid, PACKAGE_STRING); > > I am OK with lines (a bit) longer than 80 chars when it increases readability. > > > + return status; > > +} > > + > > +static grub_err_t > > +set_loader_device_part_uuid (void) > > +{ > > + grub_err_t status = GRUB_ERR_NONE; > > + char *device_name = NULL; > > + grub_device_t device; > > + char *part_uuid = NULL; > > + > > + device_name = machine_get_bootdevice (); > > + if (!device_name) > > + { > > + return grub_error (GRUB_ERR_BAD_DEVICE, > > + N_("Unable to find boot device")); > > + } > > + > > + device = grub_device_open (device_name); > > + if (!device) > > + { > > + status = grub_errno; > > + grub_dprintf (MODNAME, "Error opening device: %s", device_name); > > + goto err; > > s/err/fail/ and below please... > > > + } > > + > > + status = get_part_uuid (device, &part_uuid); > > + > > + grub_device_close (device); > > + > > + if (status == GRUB_ERR_NONE) > > + { > > + status = set_efi_str_variable ("LoaderDevicePartUUID", > > + &boot_loader_interface_vendor_guid, > > + part_uuid); > > + } > > + > > +err: > > Missing space before label. > > > + grub_free (part_uuid); > > + grub_free (device_name); > > + return status; > > +} > > + > > +static grub_err_t > > +grub_cmd_boot_loader_interface (grub_extcmd_context_t ctxt __attribute__ > > ((unused)), > > + int argc __attribute__ ((unused)), > > + char **args __attribute__ ((unused))) > > +{ > > + grub_err_t status; > > + > > + status = set_loader_info (); > > + if (status != GRUB_ERR_NONE) > > + return status; > > + > > + status = set_loader_device_part_uuid (); > > + if (status != GRUB_ERR_NONE) > > + return status; > > + > > + return GRUB_ERR_NONE; > > +} > > + > > +static grub_extcmd_t cmd; > > + > > +GRUB_MOD_INIT (boot_loader_interface) > > +{ > > + grub_dprintf (MODNAME, "%s got here\n", __func__); > > + cmd = grub_register_extcmd ( > > + "boot_loader_interface", grub_cmd_boot_loader_interface, 0, NULL, > > + N_("Set EFI variables according to Boot Loader Interface spec."), > > NULL); > > This kind of formatting does not parse well... > > cmd = grub_register_extcmd ("boot_loader_interface", > grub_cmd_boot_loader_interface, 0, NULL, > N_("Set EFI variables according to Boot Loader > Interface spec."), NULL); > > Please fix similar problem everywhere. > > > +} > > + > > +GRUB_MOD_FINI (boot_loader_interface) { grub_unregister_extcmd (cmd); } > > GRUB_MOD_FINI (boot_loader_interface) > { > grub_unregister_extcmd (cmd); > } > > Daniel > > [1] > https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/spec_ctrl.c;h=e80e2a5ed1a9b61bce564f83baf95492ff17198d;hb=HEAD > [2] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments > Thanks for the review. I will send out v2 shortly. -Oliver _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel