On Wed, Mar 26, 2025 at 5:43 AM, Vladimir 'phcoder' Serbinenko
<phco...@gmail.com> wrote:
>>
>>
>>
>> +#ifdef GRUB_MACHINE_EFI
>> +#include <grub/efi/efi.h>
>> +#include <grub/efi/disk.h>
>> +#include <grub/efi/pe32.h>
>> +#endif
>> +
>>
> Can UKI work without EFI? I think of scenario of putting e.g. EFI disk into
> coreboot or BIOS machine.
No UKI only works EFI systems.
>
>> GRUB_MOD_LICENSE ("GPLv3+");
>>
>> #define GRUB_BLS_CONFIG_PATH "/loader/entries/"
>> +#define GRUB_UKI_CONFIG_PATH "/EFI/Linux"
>> +
>> +#define GRUB_BLS_CMD 1
>> +#define GRUB_UKI_CMD 2
>> +
>> +static int cmd_type = 0;
>>
> Can we make this into an enum?
Sure thing!
>
>>
>>
>>
>> + if (pe->optional_header.magic != GRUB_PE32_NATIVE_MAGIC)
>> + {
>> + err = grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "non-native image
>> not supported");
>>
> Maybe it's a bad kernel and not not implemented yet? Later indicates that
> sick a config is valid, just not supported yet. Is it so?
Ah. I copied this error from a comparison that checks the same magic number in
grub-core/loader/efi/linux.c. I'll adjust this error statement.
>
>> +
>> +static char *
>> +uki_read_osrel (char *content, grub_off_t *pos, char **key_ret, char
>> **val_ret)
>> +{
>> + char *line;
>> + char *value;
>> + grub_size_t linelen;
>> +
>> + skip:
>> + line = content + *pos;
>> + if (*line == '\0')
>> + return NULL;
>> +
>> + linelen = 0;
>> + while (line[linelen] != '\0' && !grub_strchr ("\n\r", line[linelen]))
>>
> While I recognize the elegance of strchr, we don't use this trick in the
> code and it makes it more difficult to read. Can you use 2 comparisons
> instead?
Sure I'll do that instead.
>
>> + linelen++;
>> +
>> + /* Move pos to the next line */
>> + *pos += linelen;
>> + if (content[*pos] != '\0')
>> + (*pos)++;
>> +
>> + /* Skip empty line */
>> + if (linelen == 0)
>> + goto skip;
>> +
>> + line[linelen] = '\0';
>> +
>> + /* Remove leading white space */
>> + while (grub_strchr (" \t", *line))
>>
> Ditto
>
>> + {
>> + line++;
>> + linelen--;
>> + }
>> +
>> + /* Remove trailing whitespace */
>> + while (linelen > 0 && grub_strchr ("=", line[linelen - 1]))
>> + linelen--;
>>
> Comment doesn't match what really happens here. Also strchr with single
> character string makes no sense.
>
>> + line[linelen] = '\0';
>> +
>> + if (*line == '#')
>> + goto skip;
>> +
>> + /* Split key/value */
>> + value = line;
>> + while (*value != '\0' && !grub_strchr ("=", *value))
>> + value++;
>>
> Ditto
>
>> + if (*value == '\0')
>> + goto skip;
>> + *value = '\0';
>> + value++;
>> + while (*value != '\0' && grub_strchr ("=", *value))
>> + value++;
>>
> Ditto
>
>>
>> + if (grub_mul (argc + 1, sizeof (char *), &size))
>> + {
>> + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("overflow detected creating
>> argv list"));
>>
> Not worth translating. Generally the code which is only to guard against
> malicious input is not worth translating
>
>> + goto finish;
>> + }
>> + argv = grub_malloc (size);
>> + if (argv == NULL)
>> + {
>> + grub_error (GRUB_ERR_OUT_OF_MEMORY, "failed to allocate argv list");
>>
> Grub_malloc already sets err no.
>
>> + goto finish;
>> + }
>> + argv[0] = title;
>> + argv[argc] = NULL;
>> +
>> + src = grub_xasprintf ("insmod chain\n"
>>
> Why do you need insmod? Didn't automatic insmod work?
I didn't realize there was an automatic insmod. I just tested this without the
insmod call and you are correct that I don't need this.
>
>> + "chainloader (%s)%s/%s%s%s\n",
>> + entry->devid, entry->dirname,
>> + entry->filename, options ? " " : "", options ?
>> options : "");
>>
> Can we have a Linux variant for non-EFI?
>
>>
>>
>> /*
>> * If we aren't able to find BLS entries in the directory given by
>> info->dirname,
>> * we can fallback to the default location "/boot/loader/entries/" and
>> see if we
>> - * can find the files there.
>> + * can find the files there. If we can't find UKI entries, fallback to
>> + * "/boot/efi/EFI/Linux".
>> */
>>
> What's the purpose of fallback? It's not what user/script has requested. It
> needs to be at very least disableable
The fallback code was from some of the old blscfg code I was working with and
I added the UKI default directory. I'll add an option in case the user wants
this behavior.
>
>>
>>
>> + }
>> + else if (cmd_type == GRUB_UKI_CMD)
>> + {
>> +#ifdef GRUB_MACHINE_EFI
>> + grub_efi_loaded_image_t *image;
>> + image = grub_efi_get_loaded_image (grub_efi_image_handle);
>> + devid = grub_efidisk_get_device_name (image->device_handle);
>> +#endif
>>
> This uses grub image location. What about a scenario when booted from
> external drive and I want to boot into install on primary disk?
Maybe I might not understand this scenario very well, but would it be better
to load the UKI directory using the "--path" option? This bit of code is
trying to locate the EFI system partition of the default directory. I'm not
entirely sure the best way to find the default directory if it were to be on
a different drive.
>
>>
>> +static grub_err_t
>> +grub_cmd_blscfg (grub_extcmd_context_t ctxt, int argc __attribute__
>> ((unused)),
>> + char **args __attribute__ ((unused)))
>> +{
>> + cmd_type = GRUB_BLS_CMD;
>> + return blsuki_cmd (ctxt);
>>
> Do we really need a static variable? Maybe a parameter is more appropriate?
Sure I can use a parameter if that would be better.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
