Missing From:...
On Sat, Apr 12, 2025 at 03:53:09AM +0000, Alec Brown wrote:
> The BootLoaderSpec (BLS) defines a scheme where different bootloaders can
> share a format for boot items and a configuration directory that accepts
> these common configurations as drop-in files.
Please add links to the specification here.
> Signed-off-by: Peter Jones <pjo...@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javi...@redhat.com>
> Signed-off-by: Will Thompson <w...@endlessm.com>
> Signed-off-by: Alec Brown <alec.r.br...@oracle.com>
> ---
> Makefile.util.def | 16 +
> docs/grub.texi | 29 +
> grub-core/Makefile.core.def | 10 +
> grub-core/commands/blsuki.c | 1057 ++++++++++++++++++++++++++++++++
> grub-core/commands/legacycfg.c | 4 +-
> grub-core/commands/menuentry.c | 8 +-
> grub-core/lib/vercmp.c | 317 ++++++++++
> grub-core/normal/main.c | 6 +
> include/grub/lib/vercmp.h | 35 ++
> include/grub/menu.h | 15 +
> include/grub/normal.h | 2 +-
> tests/vercmp_unit_test.c | 65 ++
> 12 files changed, 1558 insertions(+), 6 deletions(-)
> create mode 100644 grub-core/commands/blsuki.c
> create mode 100644 grub-core/lib/vercmp.c
> create mode 100644 include/grub/lib/vercmp.h
> create mode 100644 tests/vercmp_unit_test.c
>
> diff --git a/Makefile.util.def b/Makefile.util.def
> index 038253b37..a911f2e0a 100644
> --- a/Makefile.util.def
> +++ b/Makefile.util.def
> @@ -1373,6 +1373,22 @@ program = {
> ldadd = '$(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
> };
>
> +program = {
> + testcase = native;
> + name = vercmp_unit_test;
> + common = tests/vercmp_unit_test.c;
> + common = tests/lib/unit_test.c;
> + common = grub-core/kern/list.c;
> + common = grub-core/kern/misc.c;
> + common = grub-core/tests/lib/test.c;
> + common = grub-core/lib/vercmp.c;
> + ldadd = libgrubmods.a;
> + ldadd = libgrubgcry.a;
> + ldadd = libgrubkern.a;
> + ldadd = grub-core/lib/gnulib/libgnu.a;
> + ldadd = '$(LIBDEVMAPPER) $(LIBZFS) $(LIBNVPAIR) $(LIBGEOM)';
> +};
> +
Please move tests to separate patch.
> program = {
> name = grub-menulst2cfg;
> mansection = 1;
> diff --git a/docs/grub.texi b/docs/grub.texi
> index d9b26fa36..ea7a13953 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -6417,6 +6417,7 @@ you forget a command, you can run the command
> @command{help}
> * background_image:: Load background image for active terminal
> * badram:: Filter out bad regions of RAM
> * blocklist:: Print a block list
> +* blscfg:: Load Boot Loader Specification menu entries
> * boot:: Start up your operating system
> * cat:: Show the contents of a file
> * clear:: Clear the screen
> @@ -6603,6 +6604,34 @@ Print a block list (@pxref{Block list syntax}) for
> @var{file}.
> @end deffn
>
>
> +@node blscfg
> +@subsection blscfg
> +
> +@deffn Command blscfg [@option{--path} dir] [@option{--enable-fallback}]
> [@option{--show-default}] [@option{--show-non-default}] [@option{--entry}
> file]
The long options have short counterparts. Why do not you list them here?
> +Load Boot Loader Specification entries into the GRUB menu.
> +
> +The @option{--path} option overrides the default path to the directory
> containing
> +the BLS entries. If this option isn't used, the default location is
> +/loader/entries in @code{$BOOT}. If no BLS entries are found, the
> +@option{--enable-fallback} option can be used to check for entries in the
> default
> +directory.
> +
> +The @option{--show-default} option allows the default boot entry to be added
> to the
> +GRUB menu from the BLS entries.
> +
> +The @option{--show-non-default} option allows non-default boot entries to be
> added to
> +the GRUB menu from the BLS entries.
> +
> +The @option{--entry} option allows specific boot entries to be added to the
> GRUB menu
> +from the BLS entries.
> +
> +The @option{--entry}, @option{--show-default}, and
> @option{--show-non-default} options
> +are used to filter which BLS entries are added to the GRUB menu. If none are
> +used, all entries in the default location or the location specified by
> @option{--path}
> +will be added to the GRUB menu.
> +@end deffn
I would add links to the specification here and give some examples of
BLS files. Additionally, I think it is worth mentioning how this integrates
with current grub.cfg and how both methods of loading system images are
related.
> @node boot
> @subsection boot
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index f70e02e69..f3b776c0a 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -845,6 +845,16 @@ module = {
> common = commands/blocklist.c;
> };
>
> +module = {
> + name = blsuki;
> + common = commands/blsuki.c;
> + common = lib/vercmp.c;
Probably this should be a part of the kernel.
> + enable = powerpc_ieee1275;
??? Really? PowerPC? IEEE 1275? I think something is off here...
> + enable = efi;
> + enable = i386_pc;
PC?
> + enable = emu;
Again, emu?
I think we should have EFI here only.
> +};
> +
> module = {
> name = boot;
> common = commands/boot.c;
> diff --git a/grub-core/commands/blsuki.c b/grub-core/commands/blsuki.c
> new file mode 100644
> index 000000000..0fb4f870a
> --- /dev/null
> +++ b/grub-core/commands/blsuki.c
> @@ -0,0 +1,1057 @@
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2025 Free Software Foundation, Inc.
> + *
> + * 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/>.
> + */
> +
> +#include <grub/list.h>
> +#include <grub/types.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/err.h>
> +#include <grub/dl.h>
> +#include <grub/extcmd.h>
> +#include <grub/i18n.h>
> +#include <grub/fs.h>
> +#include <grub/env.h>
> +#include <grub/file.h>
> +#include <grub/normal.h>
> +#include <grub/safemath.h>
> +#include <grub/lib/envblk.h>
> +#include <grub/lib/vercmp.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +#define GRUB_BLS_CONFIG_PATH "/loader/entries/"
> +
> +static const struct grub_arg_option bls_opt[] =
> + {
> + {"path", 'p', 0, "Specify path to find BLS entries.", N_("DIR"),
> ARG_TYPE_PATHNAME},
> + {"enable-fallback", 'f', 0, "Fallback to the default BLS path if --path
> fails to find BLS entries.", 0, ARG_TYPE_NONE},
> + {"show-default", 'd', 0, "Allow the default BLS entry to be added to the
> GRUB menu.", 0, ARG_TYPE_NONE},
> + {"show-non-default", 'n', 0, "Allow the non-default BLS entries to be
> added to the GRUB menu.", 0, ARG_TYPE_NONE},
> + {"entry", 'e', 0, "Allow specific BLS entries to be added to the GRUB
> menu.", N_("FILE"), ARG_TYPE_FILE},
> + {0, 0, 0, 0, 0, 0}
> + };
> +
> +struct keyval
> +{
> + const char *key;
> + char *val;
> +};
> +
> +static grub_blsuki_entry_t *entries = NULL;
> +
> +#define FOR_BLSUKI_ENTRIES(var) FOR_LIST_ELEMENTS (var, entries)
> +
> +static grub_err_t
> +blsuki_add_keyval (grub_blsuki_entry_t *entry, char *key, char *val)
> +{
> + char *k, *v;
> + struct keyval **kvs, *kv;
> + grub_size_t size;
> + int new_n = entry->nkeyvals + 1;
> +
> + if (entry->keyvals_size == 0)
> + {
> + size = sizeof (struct keyval *);
> + kvs = grub_malloc (size);
> + if (kvs == NULL)
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't allocate space for
> BLS key values");
> +
> + entry->keyvals = kvs;
> + entry->keyvals_size = size;
> + }
> + else if (entry->keyvals_size < new_n * sizeof (struct keyval *))
> + {
> + size = entry->keyvals_size * 2;
> + kvs = grub_realloc (entry->keyvals, size);
> + if (kvs == NULL)
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't reallocate space
> for BLS key values");
> +
> + entry->keyvals = kvs;
> + entry->keyvals_size = size;
> + }
> +
> + kv = grub_malloc (sizeof (struct keyval));
> + if (kv == NULL)
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't find space for new
> BLS key value");
> +
> + k = grub_strdup (key);
> + if (k == NULL)
> + {
> + grub_free (kv);
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't find space for
> new BLS key value");
> + }
> +
> + v = grub_strdup (val);
> + if (v == NULL)
> + {
> + grub_free (k);
> + grub_free (kv);
> + return grub_error (GRUB_ERR_OUT_OF_MEMORY, "couldn't find space for
> new BLS key value");
> + }
> +
> + kv->key = k;
> + kv->val = v;
> +
> + entry->keyvals[entry->nkeyvals] = kv;
> + entry->nkeyvals = new_n;
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +/*
> + * Find the value of the key named by keyname. If there are allowed to be
> + * more than one, pass a pointer to an int set to -1 the first time, and pass
> + * the same pointer through each time after, and it'll return them in sorted
> + * order as defined in the BLS fragment file.
> + */
> +static char *
> +blsuki_get_val (grub_blsuki_entry_t *entry, const char *keyname, int *last)
> +{
> + int idx, start = 0;
> + struct keyval *kv = NULL;
> + char *ret = NULL;
> +
> + if (last != NULL)
> + start = *last + 1;
> +
> + for (idx = start; idx < entry->nkeyvals; idx++)
> + {
> + kv = entry->keyvals[idx];
> +
> + if (grub_strcmp (keyname, kv->key) == 0)
> + {
> + ret = kv->val;
> + break;
> + }
> + }
> +
> + if (last != NULL)
> + {
> + if (idx == entry->nkeyvals)
> + *last = -1;
> + else
> + *last = idx;
> + }
> +
> + return ret;
> +}
> +
> +static grub_err_t
> +blsuki_add_entry (grub_blsuki_entry_t *entry)
> +{
> + grub_blsuki_entry_t *e, *last = NULL;
> + grub_err_t err;
> + int rc;
> +
> + if (entries == NULL)
> + {
> + grub_dprintf ("blsuki", "Add entry with id \"%s\"\n", entry->filename);
> + entries = entry;
> + return GRUB_ERR_NONE;
> + }
> +
> + FOR_BLSUKI_ENTRIES (e)
> + {
> + err = grub_split_vercmp (entry->filename, e->filename, true, &rc);
> + if (err != GRUB_ERR_NONE)
> + return err;
> +
> + if (rc == GRUB_VERCMP_SAME)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "duplicate file");
> +
> + if (rc == GRUB_VERCMP_NEWER)
> + {
> + grub_dprintf ("blsuki", "Add entry with id \"%s\"\n",
> entry->filename);
> + grub_list_push (GRUB_AS_LIST_P (&e), GRUB_AS_LIST (entry));
> + if (entry->next == entries)
> + {
> + entries = entry;
> + entry->prev = NULL;
> + }
> + else
> + last->next = entry;
> + return GRUB_ERR_NONE;
> + }
> + last = e;
> + }
> +
> + if (last != NULL)
> + {
> + grub_dprintf ("blsuki", "Add entry with id \"%s\"\n", entry->filename);
> + last->next = entry;
> + entry->prev = &last;
> + }
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +bls_read_entry (grub_file_t f, grub_blsuki_entry_t *entry)
> +{
> + grub_err_t err = GRUB_ERR_NONE;
> +
> + for (;;)
> + {
> + char *buf;
> + char *separator;
> +
> + buf = grub_file_getline (f);
> + if (buf == NULL)
> + break;
> +
> + while (buf != NULL && buf[0] != '\0' && (buf[0] == ' ' || buf[0] ==
> '\t'))
> + buf++;
> + if (buf[0] == '#')
> + {
> + grub_free (buf);
> + continue;
> + }
> +
> + separator = grub_strchr (buf, ' ');
> +
> + if (separator == NULL)
> + separator = grub_strchr (buf, '\t');
> +
> + if (separator == NULL || separator[1] == '\0')
> + {
> + grub_free (buf);
> + break;
> + }
> +
> + separator[0] = '\0';
> +
> + do
> + {
> + separator++;
> + }
> + while (*separator == ' ' || *separator == '\t');
> +
> + err = blsuki_add_keyval (entry, buf, separator);
> + grub_free (buf);
> + if (err != GRUB_ERR_NONE)
> + break;
> + }
> +
> + return err;
> +}
> +
> +struct read_entry_info
> +{
> + const char *devid;
> + const char *dirname;
> + grub_file_t file;
> +};
> +
What is a difference between bls_read_entry() and blsuki_read_entry()?
I think every function, except really basic ones, should have shorter or
longer descriptions what they do.
> +static int
> +blsuki_read_entry (const char *filename,
> + const struct grub_dirhook_info *dirhook_info __attribute__
> ((__unused__)),
> + void *data)
> +{
> + grub_size_t m = 0, n, ext_len = 5;
What is n? What is m?
> + grub_err_t err;
> + char *p = NULL;
> + grub_file_t f = NULL;
> + grub_blsuki_entry_t *entry;
> + struct read_entry_info *info = (struct read_entry_info *) data;
> +
> + grub_dprintf ("blsuki", "filename: \"%s\"\n", filename);
> +
> + n = grub_strlen (filename);
> +
> + if (info->file != NULL)
> + {
> + f = info->file;
> + }
> + else
> + {
> + if (filename[0] == '.')
It seems to me you are skipping all files starting with '.' in name.
Is it intentional?
> + return 0;
> +
> + if (n <= 5)
Why 5? I think it begs for comment.
> + return 0;
> +
> + if (grub_strcmp (filename + n - ext_len, ".conf") != 0)
Are you sure it does not underflow? What will happen if the file name is
shorter than ".conf"?
> + return 0;
> +
> + p = grub_xasprintf ("(%s)%s/%s", info->devid, info->dirname, filename);
> +
> + f = grub_file_open (p, GRUB_FILE_TYPE_CONFIG);
> + grub_free (p);
> + if (f == NULL)
> + goto finish;
> + }
> +
> + entry = grub_zalloc (sizeof (*entry));
> + if (entry == NULL)
> + goto finish;
> +
> + if (info->file != NULL)
> + {
> + char *slash;
> +
> + slash = grub_strrchr (filename, '/');
> + if (slash == NULL)
> + slash = grub_strrchr (filename, '\\');
> +
> + if (slash != NULL)
> + {
> + while (*slash == '/' || *slash == '\\')
> + slash++;
> +
> + m = slash - filename;
> + }
> + else
> + m = 0;
> + }
> + n -= m;
When I see this I think the functions require more comments in the code...
> + entry->filename = grub_strndup (filename + m, n);
> + if (entry->filename == NULL)
> + {
> + grub_free (entry);
> + goto finish;
> + }
> +
> + err = bls_read_entry (f, entry);
> +
> + if (err == GRUB_ERR_NONE)
> + blsuki_add_entry (entry);
> + else
> + grub_free (entry);
> +
> + finish:
> + if (f != NULL)
> + grub_file_close (f);
> +
> + return 0;
> +}
> +
> +static char **
> +blsuki_make_list (grub_blsuki_entry_t *entry, const char *key, int *num)
> +{
> + int last = -1;
Should not you define -1 as a constant?
> + char *val;
> + int nlist = 0;
> + char **list;
> +
> + list = grub_malloc (sizeof (char *));
> + if (list == NULL)
> + return NULL;
> + list[0] = NULL;
If you use grub_zalloc() you can drop this NULL assignment.
> + while (1)
> + {
> + char **new;
> +
> + val = blsuki_get_val (entry, key, &last);
> + if (val == NULL)
> + break;
> +
> + new = grub_realloc (list, (nlist + 2) * sizeof (char *));
> + if (new == NULL)
> + break;
> +
> + list = new;
> + list[nlist++] = val;
> + list[nlist] = NULL;
Ditto.
> + }
> +
> + if (nlist == 0)
> + {
> + grub_free (list);
> + return NULL;
> + }
> +
> + if (num != NULL)
> + *num = nlist;
> +
> + return list;
> +}
> +
> +static char *
> +blsuki_field_append (bool is_env_var, char *buffer, const char *start, const
> char *end)
> +{
> + char *tmp;
> + const char *field;
> + int term = (is_env_var == true) ? 2 : 1;
> + grub_size_t size = 0;
> +
> + tmp = grub_strndup (start, end - start + 1);
> + if (tmp == NULL)
> + return NULL;
> +
> + field = tmp;
> +
> + if (is_env_var == true)
> + {
> + field = grub_env_get (tmp);
> + if (field == NULL)
> + return buffer;
> + }
> +
> + if (grub_add (grub_strlen (field), term, &size))
> + return NULL;
> +
> + if (buffer == NULL)
> + buffer = grub_zalloc (size);
> + else
> + {
> + if (grub_add (size, grub_strlen (buffer), &size))
> + return NULL;
> + buffer = grub_realloc (buffer, size);
> + }
> +
> + if (buffer == NULL)
> + return NULL;
> +
> + tmp = buffer + grub_strlen (buffer);
> + tmp = grub_stpcpy (tmp, field);
> +
> + if (is_env_var == true)
> + tmp = grub_stpcpy (tmp, " ");
Wrong indention.
> + return buffer;
> +}
> +
> +static char *
> +blsuki_expand_val (const char *value)
> +{
> + char *buffer = NULL;
> + const char *start = value;
> + const char *end = value;
> + bool is_env_var = false;
> +
> + if (value == NULL)
> + return NULL;
> +
> + while (*value != '\0')
> + {
> + if (*value == '$')
> + {
> + if (start != end)
> + {
> + buffer = blsuki_field_append (is_env_var, buffer, start, end);
> + if (buffer == NULL)
> + return NULL;
> + }
> +
> + is_env_var = true;
> + start = value + 1;
> + }
> + else if (is_env_var == true)
> + {
> + if (grub_isalnum (*value) == 0 && *value != '_')
> + {
> + buffer = blsuki_field_append (is_env_var, buffer, start, end);
> + is_env_var = false;
> + start = value;
> + if (*start == ' ')
> + start++;
> + }
> + }
> +
> + end = value;
> + value++;
> + }
> +
> + if (start != end)
> + {
> + buffer = blsuki_field_append (is_env_var, buffer, start, end);
> + if (buffer == NULL)
> + return NULL;
> + }
> +
> + return buffer;
> +}
> +
> +static char **
> +blsuki_early_initrd_list (const char *initrd)
> +{
> + int nlist = 0;
> + char **list = NULL;
> + char *separator;
> + char *tmp;
> +
> + while ((separator = grub_strchr (initrd, ' ')))
What about TABs as separators? Are they allowed? I think they are...
> + {
> + list = grub_realloc (list, (nlist + 2) * sizeof (char *));
> + if (list == NULL)
> + return NULL;
> +
> + tmp = grub_strndup (initrd, separator - initrd);
> + if (tmp == NULL)
> + {
> + grub_free (list);
> + return NULL;
> + }
> + list[nlist++] = tmp;
> + list[nlist] = NULL;
> + initrd = separator + 1;
> + }
> +
> + list = grub_realloc (list, (nlist + 2) * sizeof (char *));
> + if (list == NULL)
> + return NULL;
> +
> + tmp = grub_strndup (initrd, grub_strlen (initrd));
> + if (tmp == NULL)
> + {
> + grub_free (list);
> + return NULL;
> + }
> + list[nlist++] = tmp;
> + list[nlist] = NULL;
> +
> + return list;
> +}
> +
> +static void
> +bls_create_entry (grub_blsuki_entry_t *entry)
> +{
> + int argc = 0;
> + const char **argv = NULL;
> + char *title = NULL;
> + char *clinux = NULL;
> + char *linux_path = NULL;
> + grub_size_t linux_size;
> + char *options = NULL;
> + char **initrds = NULL;
> + char *initrd = NULL;
> + grub_size_t initrd_size;
> + const char *early_initrd = NULL;
> + char **early_initrds = NULL;
> + char *initrd_prefix = NULL;
> + char *prefix = NULL;
> + char *devicetree = NULL;
> + char *dt = NULL;
> + grub_size_t dt_size;
> + char *id = entry->filename;
> + char *dotconf = id;
> + char *hotkey = NULL;
> + char *users = NULL;
> + char **classes = NULL;
> + char **args = NULL;
> + char *src = NULL;
> + char *tmp;
> + const char *sdval = NULL;
Do we need all these variables?
> + int i;
> + grub_size_t size = 0;
> + bool add_dt_prefix = false;
> + bool savedefault;
> +
> + linux_path = blsuki_get_val (entry, "linux", NULL);
> + if (linux_path == NULL)
> + {
> + grub_dprintf ("blsuki", "Skipping file %s with no 'linux' key.\n",
> entry->filename);
> + goto finish;
> + }
> +
> + if (grub_add (sizeof ("linux "), grub_strlen (linux_path), &linux_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> linux buffer size");
> + goto finish;
> + }
> + clinux = grub_malloc (linux_size);
> + if (clinux == NULL)
> + goto finish;
> + tmp = clinux;
> + tmp = grub_stpcpy (tmp, "linux");
> + tmp = grub_stpcpy (tmp, " ");
> + tmp = grub_stpcpy (tmp, linux_path);
> +
> + /* Strip the ".conf" off the end before we make it our "id" field. */
> + do
> + {
> + dotconf = grub_strstr (dotconf, ".conf");
> + }
> + while (dotconf != NULL && dotconf[5] != '\0');
> + if (dotconf != NULL)
> + dotconf[0] = '\0';
This code looks strange. Why do not you compare 5 last characters of the
string with ".conf" and cut it when it matches?
> + title = blsuki_get_val (entry, "title", NULL);
> + options = blsuki_expand_val (blsuki_get_val (entry, "options", NULL));
> +
> + if (options == NULL)
> + options = blsuki_expand_val (grub_env_get ("default_kernelopts"));
> +
> + initrds = blsuki_make_list (entry, "initrd", NULL);
> +
> + devicetree = blsuki_expand_val (blsuki_get_val (entry, "devicetree",
> NULL));
> +
> + if (devicetree == NULL)
> + {
> + devicetree = blsuki_expand_val (grub_env_get ("devicetree"));
> + add_dt_prefix = true;
> + }
> +
> + hotkey = blsuki_get_val (entry, "grub_hotkey", NULL);
> + users = blsuki_expand_val (blsuki_get_val (entry, "grub_users", NULL));
> + classes = blsuki_make_list (entry, "grub_class", NULL);
> + args = blsuki_make_list (entry, "grub_arg", &argc);
> +
> + argc += 1;
> + if (grub_mul (argc + 1, sizeof (char *), &size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, N_("overflow detected creating argv
> list"));
> + goto finish;
> + }
> + argv = grub_malloc (size);
> + if (argv == NULL)
> + goto finish;
> + argv[0] = title ? title : linux_path;
> + for (i = 1; i < argc; i++)
> + argv[i] = args[i-1];
> + argv[argc] = NULL;
> +
> + early_initrd = grub_env_get ("early_initrd");
> +
> + grub_dprintf ("blsuki", "adding menu entry for \"%s\" with id \"%s\"\n",
> + title, id);
> + if (early_initrd != NULL)
> + {
> + early_initrds = blsuki_early_initrd_list (early_initrd);
> + if (early_initrds == NULL)
> + {
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to create early initrd
> list"));
> + goto finish;
> + }
> +
> + if (initrds != NULL && initrds[0] != NULL)
> + {
> + initrd_prefix = grub_strrchr (initrds[0], '/');
grub_strrchr() may return NULL...
> + initrd_prefix = grub_strndup (initrds[0], initrd_prefix - initrds[0]
> + 1);
> + }
> + else
> + {
> + initrd_prefix = grub_strrchr (linux_path, '/');
Ditto...
> + initrd_prefix = grub_strndup (linux_path, initrd_prefix - linux_path
> + 1);
> + }
> +
> + if (initrd_prefix == NULL)
> + {
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to allocate initrd
> prefix buffer"));
> + goto finish;
> + }
> + }
> +
> + if (early_initrds != NULL || initrds != NULL)
> + {
> + initrd_size = sizeof ("initrd");
> +
> + for (i = 0; early_initrds != NULL && early_initrds[i] != NULL; i++)
> + {
> + if (grub_add (initrd_size, sizeof (" "), &initrd_size) ||
> + grub_add (initrd_size, grub_strlen (initrd_prefix), &initrd_size)
> ||
> + grub_add (initrd_size, grub_strlen (early_initrds[i]),
> &initrd_size) ||
> + grub_add (initrd_size, 1, &initrd_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> initrd buffer size");
> + goto finish;
> + }
> + }
> +
> + for (i = 0; initrds != NULL && initrds[i] != NULL; i++)
> + {
> + if (grub_add (initrd_size, sizeof (" "), &initrd_size) ||
You know you add 2 here...
> + grub_add (initrd_size, grub_strlen (initrds[i]), &initrd_size) ||
> + grub_add (initrd_size, 1, &initrd_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> initrd buffer size");
> + goto finish;
> + }
> + }
> +
> + if (grub_add (initrd_size, 1, &initrd_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> initrd buffer size");
> + goto finish;
> + }
> +
> + initrd = grub_malloc (initrd_size);
> + if (initrd == NULL)
> + goto finish;
> +
> + tmp = grub_stpcpy (initrd, "initrd");
> + for (i = 0; early_initrds != NULL && early_initrds[i] != NULL; i++)
> + {
> + grub_dprintf ("blsuki", "adding early initrd %s\n", early_initrds[i]);
> + tmp = grub_stpcpy (tmp, " ");
> + tmp = grub_stpcpy (tmp, initrd_prefix);
> + tmp = grub_stpcpy (tmp, early_initrds[i]);
> + grub_free (early_initrds[i]);
> + }
> +
> + for (i = 0; initrds != NULL && initrds[i] != NULL; i++)
> + {
> + grub_dprintf ("blsuki", "adding initrd %s\n", initrds[i]);
> + tmp = grub_stpcpy (tmp, " ");
> + tmp = grub_stpcpy (tmp, initrds[i]);
> + }
> + tmp = grub_stpcpy (tmp, "\n");
> + }
> +
> + if (devicetree != NULL)
> + {
> + if (add_dt_prefix == true)
> + {
> + prefix = grub_strrchr (linux_path, '/');
> + prefix = grub_strndup (linux_path, prefix - linux_path + 1);
> + if (prefix == NULL)
> + {
> + grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to allocate prefix
> buffer"));
> + goto finish;
> + }
> + }
> +
> + if (grub_add (sizeof ("devicetree "), grub_strlen (devicetree),
> &dt_size) ||
> + grub_add (dt_size, 1, &dt_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> device tree buffer size");
> + goto finish;
> + }
> +
> + if (add_dt_prefix == true)
> + {
> + if (grub_add (dt_size, grub_strlen (prefix), &dt_size))
> + {
> + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> device tree buffer size");
> + goto finish;
> + }
> + }
> +
> + dt = grub_malloc (dt_size);
> + if (dt == NULL)
> + goto finish;
> + tmp = dt;
> + tmp = grub_stpcpy (dt, "devicetree");
> + tmp = grub_stpcpy (tmp, " ");
> + if (add_dt_prefix == true)
> + tmp = grub_stpcpy (tmp, prefix);
> + tmp = grub_stpcpy (tmp, devicetree);
> + tmp = grub_stpcpy (tmp, "\n");
> +
> + grub_free (prefix);
> + }
> +
> + grub_dprintf ("blsuki", "devicetree %s for id:\"%s\"\n", dt, id);
> +
> + sdval = grub_env_get ("save_default");
> + savedefault = ((NULL != sdval) && (grub_strcmp (sdval, "true") == 0));
> + src = grub_xasprintf ("%s%s%s%s\n"
> + "%s%s",
> + savedefault ? "savedefault\n" : "",
> + clinux, options ? " " : "", options ? options : "",
> + initrd ? initrd : "", dt ? dt : "");
> +
> + grub_normal_add_menu_entry (argc, argv, classes, id, users, hotkey, NULL,
> src, 0, entry);
I think I would split this function to smaller parts for readability...
> + finish:
> + grub_free (clinux);
> + grub_free (dt);
> + grub_free (initrd);
> + grub_free (initrd_prefix);
> + grub_free (early_initrds);
> + grub_free (devicetree);
> + grub_free (initrds);
> + grub_free (options);
> + grub_free (classes);
> + grub_free (args);
> + grub_free (argv);
> + grub_free (src);
> +}
> +
> +struct find_entry_info
> +{
> + const char *dirname;
> + const char *devid;
> + grub_device_t dev;
> + grub_fs_t fs;
> +};
> +
> +static grub_err_t
> +blsuki_set_find_entry_info (struct find_entry_info *info, const char
> *dirname, const char *devid)
> +{
> + grub_device_t dev;
> + grub_fs_t fs;
> +
> + if (info == NULL)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "info parameter is not set");
> +
> + if (devid == NULL)
> + {
> +#ifdef GRUB_MACHINE_EMU
> + devid = "host";
> +#else
> + devid = grub_env_get ("root");
> +#endif
Is the EMU implementation fully functional? If yes I would move it to
separate patch to not confuse a reader. If not I would drop it...
> + if (devid == NULL)
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("variable `%s' isn't
> set"), "root");
> + }
> +
> + /* Check that we aren't closing and opening the same device. */
> + if (info->dev != NULL && grub_strcmp (info->devid, devid) != 0)
> + {
> + grub_device_close (info->dev);
> + info->dev = NULL;
> + }
> + /* If we are using the same device, then we can skip this step and only
> set the directory. */
> + if (info->dev == NULL)
> + {
> + grub_dprintf ("blsuki", "opening %s\n", devid);
> + dev = grub_device_open (devid);
> + if (dev == NULL)
> + return grub_errno;
> +
> + grub_dprintf ("blsuki", "probing fs\n");
> + fs = grub_fs_probe (dev);
> + if (fs == NULL)
> + {
> + grub_device_close (dev);
> + return grub_errno;
> + }
> +
> + info->devid = devid;
> + info->dev = dev;
> + info->fs = fs;
> + }
> +
> + info->dirname = dirname;
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +/* info: the filesystem object the file is on. */
> +static void
> +blsuki_find_entry (struct find_entry_info *info, bool enable_fallback)
> +{
> + struct read_entry_info read_entry_info;
> + grub_fs_t dir_fs = NULL;
> + grub_device_t dir_dev = NULL;
> + int fallback = 0;
s/int/bool/
> + int r = 0;
You can drop this assignment.
> + read_fallback:
No, please use for/while for the loop here...
> + read_entry_info.file = NULL;
> + read_entry_info.dirname = info->dirname;
> +
> + grub_dprintf ("blsuki", "scanning dir: %s\n", info->dirname);
> + dir_dev = info->dev;
> + dir_fs = info->fs;
> + read_entry_info.devid = info->devid;
> +
> + r = dir_fs->fs_dir (dir_dev, read_entry_info.dirname, blsuki_read_entry,
> + &read_entry_info);
> + if (r != 0)
> + {
> + grub_dprintf ("blsuki", "blsuki_read_entry returned error\n");
> + grub_errno = GRUB_ERR_NONE;
> + }
> +
> + /*
> + * 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.
> + */
> + if (entries == NULL && fallback == 0 && enable_fallback == true)
> + {
> + blsuki_set_find_entry_info (info, GRUB_BLS_CONFIG_PATH, NULL);
> + grub_dprintf ("blsuki", "Entries weren't found in %s, fallback to
> %s\n",
> + read_entry_info.dirname, info->dirname);
> + fallback = 1;
> + goto read_fallback;
> + }
> +}
> +
> +static grub_err_t
> +blsuki_load_entries (char *path, bool enable_fallback)
> +{
> + grub_size_t len;
> + static grub_err_t r;
> + const char *devid = NULL;
> + char *dir = NULL;
> + struct find_entry_info info = {
> + .dev = NULL,
> + .fs = NULL,
> + .dirname = NULL,
> + };
> + struct read_entry_info rei = {
> + .devid = NULL,
> + .dirname = NULL,
> + };
> +
> + if (path != NULL)
> + {
> + len = grub_strlen (path);
> + if (grub_strcmp (path + len - 5, ".conf") == 0)
What if len < 5?
> + {
> + rei.file = grub_file_open (path, GRUB_FILE_TYPE_CONFIG);
> + if (rei.file == NULL)
> + return grub_errno;
> +
> + /* blsuki_read_entry() closes the file. */
> + return blsuki_read_entry (path, NULL, &rei);
> + }
> + else if (path[0] == '(')
> + {
> + devid = path + 1;
> +
> + dir = grub_strchr (path, ')');
> + if (dir == NULL)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid file name
> `%s'"), path);
> +
> + *dir = '\0';
> +
> + /* Check if there is more than the devid in the path. */
> + if (dir + 1 < path + len)
> + dir = dir + 1;
> + }
> + else if (path[0] == '/')
> + dir = path;
> + }
> +
> + if (dir == NULL)
> + dir = (char *) GRUB_BLS_CONFIG_PATH;
> +
> + r = blsuki_set_find_entry_info (&info, dir, devid);
> + if (r == GRUB_ERR_NONE)
> + blsuki_find_entry (&info, enable_fallback);
> +
> + if (info.dev != NULL)
> + grub_device_close (info.dev);
> +
> + return r;
> +}
> +
> +static bool
> +blsuki_is_default_entry (const char *def_entry, grub_blsuki_entry_t *entry,
> int idx)
> +{
> + const char *title;
> + int def_idx;
> +
> + if (def_entry == NULL)
> + return false;
> +
> + if (grub_strcmp (def_entry, entry->filename) == 0)
> + return true;
> +
> + title = blsuki_get_val (entry, "title", NULL);
> +
> + if (title != NULL && grub_strcmp (def_entry, title) == 0)
> + return true;
> +
> + def_idx = (int) grub_strtol (def_entry, NULL, 0);
> + if (grub_errno != GRUB_ERR_NONE)
This check is unreliable. Please take a look at commit ac8a37dda (net/http:
Allow use of non-standard TCP/IP ports) how it should be done properly.
> + {
> + grub_errno = GRUB_ERR_NONE;
> + return false;
> + }
> +
> + if (def_idx == idx)
> + return true;
> +
> + return false;
> +}
> +
> +static grub_err_t
> +blsuki_create_entries (bool show_default, bool show_non_default, char
> *entry_id)
> +{
> + const char *def_entry = NULL;
> + grub_blsuki_entry_t *entry = NULL;
> + int idx = 0;
> +
> + def_entry = grub_env_get ("default");
> +
> + FOR_BLSUKI_ENTRIES(entry)
> + {
> + if (entry->visible == 1)
> + {
> + idx++;
> + continue;
> + }
> + if ((show_default == true && blsuki_is_default_entry (def_entry,
> entry, idx) == true) ||
> + (show_non_default == true && blsuki_is_default_entry (def_entry,
> entry, idx) == false) ||
> + (entry_id != NULL && grub_strcmp (entry_id, entry->filename) == 0))
> + {
> + bls_create_entry (entry);
> + entry->visible = 1;
> + }
> + idx++;
> + }
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +static grub_err_t
> +grub_cmd_blscfg (grub_extcmd_context_t ctxt, int argc __attribute__
> ((unused)),
> + char **args __attribute__ ((unused)))
> +{
> + grub_err_t err;
> + struct grub_arg_list *state = ctxt->state;
> + char *path = NULL;
> + char *entry_id = NULL;
> + bool enable_fallback = false;
> + bool show_default = false;
> + bool show_non_default = false;
> + bool all = true;
> +
> + if (state[0].set)
> + path = state[0].arg;
> + if (state[1].set)
> + enable_fallback = true;
> + if (state[2].set)
> + {
> + show_default = true;
> + all = false;
> + }
> + if (state[3].set)
> + {
> + show_non_default = true;
> + all = false;
> + }
> + if (state[4].set)
> + {
> + entry_id = state[4].arg;
> + all = false;
> + }
> + if (all == true)
> + {
> + show_default = true;
> + show_non_default = true;
> + }
> +
> + err = blsuki_load_entries (path, enable_fallback);
> + if (err != GRUB_ERR_NONE)
> + return err;
> +
> + return blsuki_create_entries (show_default, show_non_default, entry_id);
> +}
> +
> +static grub_extcmd_t bls_cmd;
> +
> +GRUB_MOD_INIT(blsuki)
> +{
> + bls_cmd = grub_register_extcmd ("blscfg", grub_cmd_blscfg, 0,
> + N_("[-p|--path] [-f|--enable-fallback] DIR
> [-d|--show-default] [-n|--show-non-default] [-e|--entry] FILE"),
> + N_("Import Boot Loader Specification
> snippets."),
> + bls_opt);
> +}
> +
> +GRUB_MOD_FINI(blsuki)
> +{
> + grub_unregister_extcmd (bls_cmd);
> +}
> diff --git a/grub-core/commands/legacycfg.c b/grub-core/commands/legacycfg.c
> index 3bf9fe2e4..f3c86dc7f 100644
> --- a/grub-core/commands/legacycfg.c
> +++ b/grub-core/commands/legacycfg.c
> @@ -143,7 +143,7 @@ legacy_file (const char *filename)
> args[0] = oldname;
> grub_normal_add_menu_entry (1, args, NULL, NULL, "legacy",
> NULL, NULL,
> - entrysrc, 0);
> + entrysrc, 0, NULL);
> grub_free (args);
> entrysrc[0] = 0;
> grub_free (oldname);
> @@ -204,7 +204,7 @@ legacy_file (const char *filename)
> }
> args[0] = entryname;
> grub_normal_add_menu_entry (1, args, NULL, NULL, NULL,
> - NULL, NULL, entrysrc, 0);
> + NULL, NULL, entrysrc, 0, NULL);
> grub_free (args);
> }
>
> diff --git a/grub-core/commands/menuentry.c b/grub-core/commands/menuentry.c
> index 720e6d8ea..09749c415 100644
> --- a/grub-core/commands/menuentry.c
> +++ b/grub-core/commands/menuentry.c
> @@ -78,7 +78,7 @@ grub_normal_add_menu_entry (int argc, const char **args,
> char **classes, const char *id,
> const char *users, const char *hotkey,
> const char *prefix, const char *sourcecode,
> - int submenu)
> + int submenu, grub_blsuki_entry_t *blsuki)
> {
> int menu_hotkey = 0;
> char **menu_args = NULL;
> @@ -188,6 +188,7 @@ grub_normal_add_menu_entry (int argc, const char **args,
> (*last)->args = menu_args;
> (*last)->sourcecode = menu_sourcecode;
> (*last)->submenu = submenu;
> + (*last)->blsuki = blsuki;
>
> menu->size++;
> return GRUB_ERR_NONE;
> @@ -286,7 +287,8 @@ grub_cmd_menuentry (grub_extcmd_context_t ctxt, int argc,
> char **args)
> users,
> ctxt->state[2].arg, 0,
> ctxt->state[3].arg,
> - ctxt->extcmd->cmd->name[0] == 's');
> + ctxt->extcmd->cmd->name[0] == 's',
> + NULL);
>
> src = args[argc - 1];
> args[argc - 1] = NULL;
> @@ -303,7 +305,7 @@ grub_cmd_menuentry (grub_extcmd_context_t ctxt, int argc,
> char **args)
> ctxt->state[0].args, ctxt->state[4].arg,
> users,
> ctxt->state[2].arg, prefix, src + 1,
> - ctxt->extcmd->cmd->name[0] == 's');
> + ctxt->extcmd->cmd->name[0] == 's', NULL);
>
> src[len - 1] = ch;
> args[argc - 1] = src;
> diff --git a/grub-core/lib/vercmp.c b/grub-core/lib/vercmp.c
> new file mode 100644
> index 000000000..726e2321b
> --- /dev/null
> +++ b/grub-core/lib/vercmp.c
I would move this code to separate patch. Even if it will be unused initially.
> @@ -0,0 +1,317 @@
> +/*
> + * GRUB -- GRand Unified Bootloader
> + * Copyright (C) 2025 Free Software Foundation, Inc.
> + *
> + * 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/>.
> + */
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/err.h>
> +#include <grub/lib/vercmp.h>
> +
> +#define GOTO_RETURN(x) ({ *ret = (x); goto finish; })
> +
> +/*
> + * compare alpha and numeric segments of two versions
> + * return 1: a is newer than b
> + * 0: a and b are the same version
> + * -1: a is older than b
> + */
> +grub_err_t
> +grub_vercmp (const char *a, const char *b, int *ret)
Where is this algorithm come from? I suppose you copied it from
somewhere. If yes please mention source here. If not I think you
should copy the code from somewhere to not invent the wheel.
> +{
> + char oldch1, oldch2;
> + char *abuf, *bbuf;
> + char *str1, *str2;
> + char *one, *two;
> + int rc;
> + bool isnum;
> +
> + if (ret == NULL)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("return parameter is not
> set"));
> + *ret = 0;
> +
> + grub_dprintf ("blscfg", "%s comparing %s and %s\n", __func__, a, b);
This is generic thing not BLS one... And it seems to me this message is
not very helpful...
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
