>
>
>
> +#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.
> 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?
>
>
>
> + 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?
> +
> +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?
> + 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?
+ "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
>
>
> + }
> + 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?
>
> +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?
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
