On Fri, May 30, 2025 at 2:47 AM, Avnish Chouhan <avn...@linux.ibm.com> wrote: > > Message: 1 > > Date: Wed, 21 May 2025 12:51:26 +0000 > > From: Alec Brown <alec.r.br...@oracle.com> > > To: grub-devel@gnu.org > > Cc: christopher.obb...@linaro.org, daniel.ki...@oracle.com, > > jan.setjeeil...@oracle.com, alec.r.br...@oracle.com, > > mate.ku...@canonical.com, pjo...@redhat.com, > > ross.philip...@oracle.com, 93...@debian.org, phco...@gmail.com > > Subject: [PATCH v4 4/4] blsuki: Add uki command to load Unified Kernel > > Image entries > > Message-ID: <20250521125126.3928350-5-alec.r.br...@oracle.com> > > > > A Unified Kernel Image is a single UEFI PE file that combines a UEFI > > boot stub, a Linux kernel image, an initrd, and further resources. The > > uki command will locate where the UKI file is and create a GRUB menu > > entry to load it. > > > > The Unified Kernel Image Specification: > > https://urldefense.com/v3/__https://uapi-group.org/specifications/spec > > s/unified_kernel_image/__;!!ACWV5N9M2RV99hQ!MLcCQykiFNenp16SX023gevWvP > > 4pETCVqAOq96F90j7i6HTTXoeUZAVSGOGc2rcLXQWEEwXZmF02M-W1xmoX$ > > > > Signed-off-by: Alec Brown <alec.r.br...@oracle.com> > > --- > > docs/grub.texi | 33 +++ > > grub-core/commands/blsuki.c | 463 +++++++++++++++++++++++++++++++++--- > > include/grub/menu.h | 2 + > > 3 files changed, 463 insertions(+), 35 deletions(-) > > > . > . > . > > > > +/* > > + * This function searches for the .cmdline, .osrel, and .linux > > sections of a > > + * UKI. We only need to store the data for the .cmdline and .osrel > > sections, > > + * but we also need to verify that the .linux section exists. > > + */ > > +#ifdef GRUB_MACHINE_EFI > > +static grub_err_t > > +uki_parse_keyvals (grub_file_t f, grub_blsuki_entry_t *entry) { > . > . > . > > + > > + for (int i = 0; i < coff_header->num_sections; i++) > > + { > > + key = NULL; > > + val = NULL; > > + section = grub_zalloc (sizeof (*section)); > > + if (section == NULL) > > + { > > + err = grub_errno; > > + goto finish; > > + } > > + > > + if (grub_file_seek (f, section_offset) == (grub_off_t) -1 || > > + grub_file_read (f, section, sizeof (*section)) != sizeof > > (*section)) > > + { > > + err = grub_error (GRUB_ERR_FILE_READ_ERROR, "failed to read > > section header"); > > + goto finish; > > + } > > + > > + key = grub_strndup (section->name, 8); > > + if (key == NULL) > > + { > > + err = grub_errno; > > + goto finish; > > + } > > + > > + for (int j = 0; target[j] != NULL; j++) > > + { > > + if (grub_strcmp (key, target[j]) == 0) > > + { > > + /* > > + * We don't need to read the contents of the .linux PE > > +section, > > but we > > + * should verify that the section exists. > > + */ > > + if (grub_strcmp (key, ".linux") == 0) > > + { > > + has_linux = true; > > + break; > > + } > > + > > + val = grub_zalloc (section->raw_data_size); > > + if (val == NULL) > > + { > > + err = grub_errno; > > + goto finish; > > + } > > + > > + if (grub_file_seek (f, section->raw_data_offset) == > > (grub_off_t) -1 || > > + grub_file_read (f, val, section->raw_data_size) != > > (grub_ssize_t) > > section->raw_data_size) > > + { > > + err = grub_error (GRUB_ERR_FILE_READ_ERROR, "failed to read > > section"); > > + goto finish; > > + } > > + > > + err = blsuki_add_keyval (entry, key, val); > > + if (err != GRUB_ERR_NONE) > > + goto finish; > > + > > + break; > > + } > > + } > > + > > + section_offset += sizeof (*section); > > + grub_free (section); > > + grub_free (val); > > + grub_free (key); > > + section = NULL; > > + val = NULL; > > + key = NULL; > > Hi Alec, > > These couple of NULL statements above seems redundant as you are doing the > same in the starting of > the loop! >
Hi Avnish, I moved them to the end of the loop so that we won't have to worry about a double free once the loop finishes but it looks like I forgot to remove the NULL statements at the start. I'll fix that. > And it seems there are some indentation issues in "if" conditions and "loops" > in multiple places > which probably needs to be checked/fixed. Same thing with the other patches, the indentation looks strange because tabs are being used in place of 8 spaces and the formatting of the patch looks strange because of it. > > Suggestion : Adding a new line after the loop or a condition would be good! I > see some statements > has, some doesn't. Sure thing! > > Thank you, > > Regards, > Avnish Chouhan > Thanks for looking over these patches! Alec Brown _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
