Re: [PATCH v4 2/4] blsuki: Add blscfg command to parse Boot Loader Specification snippets

[Daniel Kiper via Grub-devel <grub-devel@gnu.org>]

On Wed, May 21, 2025 at 12:51:24PM +0000, Alec Brown wrote:
> From: Peter Jones <pjones@redhat.com>
>
> 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.
>
> The BLS Specification:
> https://uapi-group.org/specifications/specs/boot_loader_specification/
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Will Thompson <wjt@endlessm.com>
> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> ---
>  docs/grub.texi                 |   47 ++
>  grub-core/Makefile.core.def    |   10 +
>  grub-core/commands/blsuki.c    | 1198 ++++++++++++++++++++++++++++++++
>  grub-core/commands/legacycfg.c |    4 +-
>  grub-core/commands/menuentry.c |    8 +-
>  grub-core/normal/main.c        |    6 +
>  include/grub/menu.h            |   15 +
>  include/grub/normal.h          |    2 +-
>  8 files changed, 1284 insertions(+), 6 deletions(-)
>  create mode 100644 grub-core/commands/blsuki.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index d9b26fa36..adab93668 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,52 @@ Print a block list (@pxref{Block list syntax}) for @var{file}.
>  @end deffn
>
>
> +@node blscfg
> +@subsection blscfg
> +
> +@deffn Command blscfg [@option{-p|--path} dir] [@option{-f|--enable-fallback}] [@option{-d|--show-default}] [@option{-n|--show-non-default}] [@option{-e|--entry} file]
> +Load Boot Loader Specification (BLS) entries into the GRUB menu. Boot entries
> +generated from @command{blscfg} won't interfere with entries from @file{grub.cfg} appearing in
> +the GRUB menu. Also, entries generated from @command{blscfg} only generate in memory and

s/only generate/exist only/

> +don't update @file{grub.cfg}.
> +
> +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

What is "$BOOT"? AIUI this comes from BLS spec. Though it has to be
clearly explained or BLS spec properly referenced here.

> +@option{--enable-fallback} option can be used to check for entries in the default
> +directory.

What is a 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.
> +
> +A BLS config file example:
> +@example
> +# /boot/loader/entries/6a9857a393724b7a981ebb5b8495b9ea-3.8.0-2.fc19.x86_64.conf
> +title        Fedora 19 (Rawhide)
> +sort-key     fedora
> +machine-id   6a9857a393724b7a981ebb5b8495b9ea
> +version      3.8.0-2.fc19.x86_64
> +options      root=UUID=6d3376e4-fc93-4509-95ec-a21d68011da2 quiet
> +architecture x64
> +linux        /6a9857a393724b7a981ebb5b8495b9ea/3.8.0-2.fc19.x86_64/linux
> +initrd       /6a9857a393724b7a981ebb5b8495b9ea/3.8.0-2.fc19.x86_64/initrd
> +@end example
> +
> +References: @uref{https://uapi-group.org/specifications/specs/boot_loader_specification/, The Boot Loader Specification}

Nice, but it has to be clearly referenced from the text above.

> +@end deffn
> +
> +
>  @node boot
>  @subsection boot
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index f70e02e69..67628f65f 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 = kern/vercmp.c;

I think this should not be a part of the module but the kernel.

> +  enable = powerpc_ieee1275;
> +  enable = efi;
> +  enable = i386_pc;
> +  enable = emu;
> +};
> +
>  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..2ad960ae3
> --- /dev/null
> +++ b/grub-core/commands/blsuki.c
> @@ -0,0 +1,1198 @@
> +/*
> + *  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/vercmp.h>
> +#include <grub/lib/envblk.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)
> +
> +/*
> + * This function will add a new keyval pair to a list of keyvals stored in the
> + * entry parameter.
> + */
> +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

s/int/last/?

> + * 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;

start = (last != NULL) ? (*last + 1) : 0;

> +  struct keyval *kv = NULL;
> +  char *ret = NULL;
> +
> +  if (last != NULL)
> +    start = *last + 1;

... and you can drop this then...

> +  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;
> +}
> +
> +/*
> + * Add a new grub_blsuki_entry_t struct to the entries list and sort it's
> + * position on the list.
> + */
> +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");

I would add file name to the error message... Though I am not sure how
this error can be triggered...

> +      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;
> +}
> +
> +/*
> + * This function parses each line of a BLS config file to obtain the key value
> + * pairs that will be used to setup the GRUB menu entries. The key value pair
> + * will be stored in a list in the entry parameter.
> + */
> +static grub_err_t
> +bls_parse_keyvals (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 != NULL" seems redundant here...

And I think buf[0] is confusing here and should be replaced with *buf.

> +	buf++;
> +      if (buf[0] == '#')

Ditto.

> +	{
> +	  grub_free (buf);

You do not free what you want here...

> +	  continue;
> +	}
> +
> +      separator = grub_strchr (buf, ' ');
> +
> +      if (separator == NULL)
> +	separator = grub_strchr (buf, '\t');
> +
> +      if (separator == NULL || separator[1] == '\0')

Hmmm... Could not you implement grub_strtok()? "man strtok" is your friend.
Copy from gnulib?

> +	{
> +	  grub_free (buf);
> +	  break;
> +	}
> +
> +      separator[0] = '\0';

Even if 'separator[0]' work *separator is more appropriate here.

> +      do
> +	{
> +	  separator++;
> +	}
> +      while (*separator == ' ' || *separator == '\t');
> +
> +      err = blsuki_add_keyval (entry, buf, separator);
> +      grub_free (buf);

Again, buf may point to not what you expect...

> +      if (err != GRUB_ERR_NONE)
> +	break;
> +    }
> +
> +  return err;
> +}
> +
> +struct read_entry_info
> +{
> +  const char *devid;
> +  const char *dirname;
> +  grub_file_t file;
> +};

I would prefer if you define all structs and variables at the beginning
of the file.

> +/*
> + * If a file hasn't already been opened, this function opens a BLS config file
> + * and initializes entry data before parsing keyvals and adding the entry to
> + * the list of BLS entries.
> + */
> +static int
> +blsuki_read_entry (const char *filename,
> +		   const struct grub_dirhook_info *dirhook_info __attribute__ ((__unused__)),
> +		   void *data)
> +{
> +  grub_size_t prefix_len = 0, filename_len, ext_len = 5;
> +  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);
> +
> +  filename_len = grub_strlen (filename);
> +
> +  if (info->file != NULL)
> +    f = info->file;
> +  else
> +    {
> +      if (filename_len < ext_len || grub_strcmp (filename + filename_len - ext_len, ".conf") != 0)

Please use 'sizeof (".conf") - 1' instead of ext_len.

> +	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 a file is opened before this function, the filename may have a prefix.

I think you mean "path" instead of "prefix" here.

Additionally, could not you use grub_get_dirname() from
grub-core/gfxmenu/gui_string_util.c or find better alternative in the
GRUB? Of course if you use grub_get_dirname() then it should be moved to
the kernel...

> +   * Since the filename is used for the ID of the GRUB menu entry, we can
> +   * remove the prefix.
> +   */
> +  if (info->file != NULL)
> +    {
> +      char *slash;
> +
> +      slash = grub_strrchr (filename, '/');
> +      if (slash == NULL)
> +	slash = grub_strrchr (filename, '\\');

I am not entirely sure why we care about "\"...

> +      if (slash != NULL)
> +	{
> +	  while (*slash == '/' || *slash == '\\')
> +	    slash++;
> +
> +	  prefix_len = slash - filename;
> +	}
> +    }
> +  filename_len -= prefix_len;
> +
> +  entry->filename = grub_strndup (filename + prefix_len, filename_len);
> +  if (entry->filename == NULL)
> +    {
> +      grub_free (entry);
> +      goto finish;
> +    }
> +
> +  err = bls_parse_keyvals (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;
> +}
> +
> +/*
> + * This function returns a list of values that had the same key in the BLS
> + * config file. The number of entries in this list is returned by the len
> + * parameter.
> + */
> +static char **
> +blsuki_make_list (grub_blsuki_entry_t *entry, const char *key, int *len)
> +{
> +  int last = -1;
> +  char *val;
> +  int nlist = 0;
> +  char **list;
> +
> +  list = grub_zalloc (sizeof (char *));
> +  if (list == NULL)
> +    return NULL;
> +
> +  while (1)
> +    {
> +      char **new;
> +
> +      /*
> +       * Since the same key might appear more than once, the 'last' variable
> +       * starts at -1 and increments to indicate the last index in the list
> +       * we obtained from blsuki_get_val().
> +       */
> +      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;

Should not we artificially limit number of values to avoid overflows?
10000?

> +      list[nlist] = NULL;
> +  }
> +
> +  if (nlist == 0)
> +    {
> +      grub_free (list);
> +      return NULL;
> +    }
> +
> +  if (len != NULL)
> +    *len = nlist;
> +
> +  return list;
> +}
> +
> +/*
> + * This function appends a field to the end of a buffer. If the field given is
> + * an enviornmental variable, it gets the value stored for that variable and
> + * appends that to the buffer instead.
> + */
> +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;

What is term?

> +  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);

This looks like GRUB extension not compatible with BLS spec. I think it
should be documented in the GRUB docs. And of course it should be
mentioned it is not compatible with other bootloaders.

> +      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, " ");
> +
> +  return buffer;
> +}
> +
> +/*
> + * This function takes a value string, checks for environmental variables, and
> + * returns the value string with all environmental variables replaced with the
> + * value stored in the variable.
> + */
> +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++;

Probably spaces should be replaced with tab here...

> +    }
> +
> +  if (start != end)
> +    {
> +      buffer = blsuki_field_append (is_env_var, buffer, start, end);
> +      if (buffer == NULL)
> +	return NULL;
> +    }
> +
> +  return buffer;
> +}
> +
> +/*
> + * This function parses a string containing initrd paths and returns it as a
> + * list.
> + */
> +static char **
> +blsuki_early_initrd_list (const char *initrd)
> +{
> +  int nlist = 0;
> +  char **list = NULL;
> +  const char *separator = initrd;
> +  char *tmp;
> +
> +  for (;;)
> +    {
> +      while (*separator != '\0' && *separator != ' ' && *separator != '\t')
> +	separator++;
> +      if (*separator == '\0')
> +	break;

grub_strtok()?

> +      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;
> +      separator++;
> +      initrd = separator;
> +    }
> +
> +  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;
> +}
> +
> +/*
> + * This function returns a string with the command to load a linux kernel with
> + * kernel command-line options based on what was specified in the BLS config
> + * file.
> + */
> +static char *
> +bls_get_linux (grub_blsuki_entry_t *entry)
> +{
> +  char *linux_path;
> +  char *linux_cmd = NULL;
> +  char *options = NULL;
> +  char *tmp;
> +  grub_size_t size;
> +
> +  linux_path = blsuki_get_val (entry, "linux", NULL);
> +  options = blsuki_expand_val (blsuki_get_val (entry, "options", NULL));
> +
> +  if (options == NULL)
> +    options = blsuki_expand_val (grub_env_get ("default_kernelopts"));

This variable has to be documented. In general all new variables have to
be documented...

> +  if (grub_add (grub_strlen ("linux "), grub_strlen (linux_path), &size))

Do you need to get into account NUL at the end of string?

s/grub_strlen ("linux ")/sizeof ("linux ")/?

Though you have to remember that sizeof() counts NUL at the end of string.

> +    {
> +      grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected while calculating linux buffer size");
> +      goto finish;
> +    }
> +
> +  if (options != NULL)
> +    {
> +      if (grub_add (size, grub_strlen (options), &size) ||
> +	  grub_add (size, 1, &size))
> +	{
> +	  grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected while calculating linux buffer size");
> +	  goto finish;
> +	}
> +    }
> +
> +  linux_cmd = grub_malloc (size);
> +  if (linux_cmd == NULL)
> +    goto finish;
> +
> +  tmp = linux_cmd;
> +  tmp = grub_stpcpy (tmp, "linux ");
> +  tmp = grub_stpcpy (tmp, linux_path);
> +  if (options != NULL)
> +    {
> +      tmp = grub_stpcpy (tmp, " ");
> +      tmp = grub_stpcpy (tmp, options);
> +    }
> +  tmp = grub_stpcpy (tmp, "\n");
> +
> + finish:
> +  grub_free (options);
> +
> +  return linux_cmd;
> +}
> +
> +/*
> + * This function returns a string with the command to load all initrds for a
> + * linux kernel image based on the list provided by the BLS config file.
> + */
> +static char *
> +bls_get_initrd (grub_blsuki_entry_t *entry)
> +{
> +  char **initrd_list;
> +  char *initrd_cmd = NULL;
> +  const char *early_initrd;
> +  char **early_initrd_list = NULL;
> +  char *tmp;
> +  char *slash;
> +  char *prefix = NULL;
> +  grub_size_t prefix_len = 0;
> +  char *linux_path;
> +  grub_size_t size;
> +  int i;
> +
> +  initrd_list = blsuki_make_list (entry, "initrd", NULL);
> +  early_initrd = grub_env_get ("early_initrd");

Again, this variable has to be documented...

> +  if (early_initrd != NULL)
> +    {
> +      early_initrd_list = blsuki_early_initrd_list (early_initrd);
> +      if (early_initrd_list == NULL)
> +	{
> +	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to create early initrd list"));
> +	  goto finish;
> +	}
> +
> +      if (initrd_list != NULL && initrd_list[0] != NULL)
> +	{
> +	  slash = grub_strrchr (initrd_list[0], '/');
> +	  if (slash == NULL)
> +	    prefix_len = slash - initrd_list[0] + 1;
> +	  prefix = grub_strndup (initrd_list[0], prefix_len);
> +	}
> +      else
> +	{
> +	  linux_path = blsuki_get_val (entry, "linux", NULL);
> +	  slash = grub_strrchr (linux_path, '/');
> +	  if (slash != NULL)
> +	    prefix_len = slash - linux_path + 1;
> +	  prefix = grub_strndup (linux_path, prefix_len);
> +	}
> +
> +      if (prefix == NULL)
> +	{
> +	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to allocate initrd prefix buffer"));
> +	  goto finish;
> +	}
> +    }
> +
> +  if (early_initrd_list != NULL || initrd_list != NULL)
> +    {
> +
> +      size = grub_strlen ("initrd");
> +
> +      for (i = 0; early_initrd_list != NULL && early_initrd_list[i] != NULL; i++)
> +	{
> +	  if (grub_add (size, 1, &size) ||
> +	      grub_add (size, grub_strlen (prefix), &size) ||
> +	      grub_add (size, grub_strlen (early_initrd_list[i]), &size))
> +	    {
> +	      grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating initrd buffer size");
> +	      goto finish;
> +	    }
> +	}
> +
> +      for (i = 0; initrd_list != NULL && initrd_list[i] != NULL; i++)
> +	{
> +	  if (grub_add (size, 1, &size) ||
> +	      grub_add (size, grub_strlen (initrd_list[i]), &size))
> +	    {
> +	      grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating initrd buffer size");
> +	      goto finish;
> +	    }
> +	}
> +
> +      if (grub_add (size, 1, &size))
> +	{
> +	  grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating initrd buffer size");
> +	  goto finish;
> +	}
> +
> +      initrd_cmd = grub_malloc (size);
> +      if (initrd_cmd == NULL)
> +	goto finish;
> +
> +      tmp = grub_stpcpy (initrd_cmd, "initrd");
> +      for (i = 0; early_initrd_list != NULL && early_initrd_list[i] != NULL; i++)
> +	{
> +	  grub_dprintf ("blsuki", "adding early initrd %s\n", early_initrd_list[i]);
> +	  tmp = grub_stpcpy (tmp, " ");
> +	  tmp = grub_stpcpy (tmp, prefix);
> +	  tmp = grub_stpcpy (tmp, early_initrd_list[i]);
> +	}
> +
> +      for (i = 0; initrd_list != NULL && initrd_list[i] != NULL; i++)
> +	{
> +	  grub_dprintf ("blsuki", "adding initrd %s\n", initrd_list[i]);
> +	  tmp = grub_stpcpy (tmp, " ");
> +	  tmp = grub_stpcpy (tmp, initrd_list[i]);
> +	}
> +      tmp = grub_stpcpy (tmp, "\n");
> +    }
> +
> + finish:
> +  for (i = 0; early_initrd_list != NULL && early_initrd_list[i] != NULL; i++)
> +    grub_free (early_initrd_list[i]);
> +  grub_free (prefix);
> +
> +  return initrd_cmd;
> +}
> +
> +/*
> + * This function returns a string with the command to load a device tree blob
> + * from the BLS config file.
> + */
> +static char *
> +bls_get_devicetree (grub_blsuki_entry_t *entry)
> +{
> +  char *dt_path;
> +  char *dt_cmd = NULL;
> +  char *tmp;
> +  char *linux_path;
> +  char *slash;
> +  char *prefix = NULL;
> +  grub_size_t prefix_len = 0;
> +  grub_size_t size;
> +  bool add_dt_prefix = false;
> +
> +  dt_path = blsuki_expand_val (blsuki_get_val (entry, "devicetree", NULL));
> +
> +  if (dt_path == NULL)
> +    {
> +      dt_path = blsuki_expand_val (grub_env_get ("devicetree"));

The GRUB documentation should be expanded for this variable.

> +      add_dt_prefix = true;
> +    }
> +
> +  if (dt_path != NULL)
> +    {
> +      if (add_dt_prefix == true)
> +	{
> +	  linux_path = blsuki_get_val (entry, "linux", NULL);
> +	  slash = grub_strchr (linux_path, '/');
> +	  if (slash != NULL)
> +	    prefix_len = slash - linux_path + 1;
> +	  prefix = grub_strndup (linux_path, prefix_len);
> +	  if (prefix == NULL)
> +	    {
> +	      grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("failed to allocate prefix buffer"));
> +	      goto finish;
> +	    }
> +	}
> +
> +      if (grub_add (grub_strlen ("devicetree "), grub_strlen (dt_path), &size) ||

s/grub_strlen ("devicetree ")/sizeof ("devicetree ") - 1/?

Please check similar places too.

> +	  grub_add (size, 1, &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 (size, grub_strlen (prefix), &size))
> +	    {
> +	      grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating device tree buffer size");
> +	      goto finish;
> +	    }
> +	}
> +      dt_cmd = grub_malloc (size);
> +      if (dt_cmd == NULL)
> +	goto finish;
> +      tmp = dt_cmd;
> +      tmp = grub_stpcpy (dt_cmd, "devicetree");
> +      tmp = grub_stpcpy (tmp, " ");
> +      if (add_dt_prefix == true)
> +        tmp = grub_stpcpy (tmp, prefix);
> +      tmp = grub_stpcpy (tmp, dt_path);
> +      tmp = grub_stpcpy (tmp, "\n");
> +    }
> +
> + finish:
> +  grub_free (prefix);
> +
> +  return dt_cmd;
> +}
> +
> +/*
> + * This function puts together all of the commands generated from the contents
> + * of the BLS config file and creates a new entry in the GRUB boot menu.
> + */
> +static void
> +bls_create_entry (grub_blsuki_entry_t *entry)
> +{
> +  int argc = 0;
> +  const char **argv = NULL;
> +  char *title = NULL;
> +  char *linux_path = NULL;
> +  char *linux_cmd = NULL;
> +  char *initrd_cmd = NULL;
> +  char *dt_cmd = NULL;
> +  char *id = entry->filename;
> +  grub_size_t id_len;
> +  char *hotkey = NULL;
> +  char *users = NULL;
> +  char **classes = NULL;
> +  char **args = NULL;
> +  char *src = NULL;
> +  const char *sdval = NULL;
> +  int i;
> +  grub_size_t size;
> +  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;
> +    }
> +
> +  id_len = grub_strlen (id);
> +  if (id_len >= 5 && grub_strcmp (id + id_len - 5, ".conf") == 0)
> +    id[id_len - 5] = '\0';

Please avoid cryptic constants. I think 5 should be replaced with
'sizeof (".conf") - 1'. Maybe it is worth defining constant for this
value because as I can see it is used in a few places. You can use
expression given above to define a constant.

> +  title = blsuki_get_val (entry, "title", NULL);
> +  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);

These smell like non-standard extensions. They beg for documentation.

> +  argc++;
> +  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;

(title != NULL) ? title : linux_path;

> +  for (i = 1; i < argc; i++)
> +    argv[i] = args[i-1];
> +  argv[argc] = NULL;
> +
> +  linux_cmd = bls_get_linux (entry);
> +  if (linux_cmd == NULL)
> +    goto finish;
> +
> +  initrd_cmd = bls_get_initrd (entry);
> +  if (grub_errno != GRUB_ERR_NONE)
> +    goto finish;
> +
> +  dt_cmd = bls_get_devicetree (entry);
> +  if (grub_errno != GRUB_ERR_NONE)
> +    goto finish;
> +
> +  sdval = grub_env_get ("save_default");
> +  savedefault = ((NULL != sdval) && (grub_strcmp (sdval, "true") == 0));
> +  src = grub_xasprintf ("%s%s%s%s",
> +			savedefault ? "savedefault\n" : "",
> +			linux_cmd, initrd_cmd ? initrd_cmd : "",
> +			dt_cmd ? dt_cmd : "");
> +
> +  grub_normal_add_menu_entry (argc, argv, classes, id, users, hotkey, NULL, src, 0, entry);
> +
> + finish:
> +  grub_free (linux_cmd);
> +  grub_free (dt_cmd);
> +  grub_free (initrd_cmd);
> +  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;
> +};

Please define this at the beginning of the file.

> +/*
> + * This function fills a find_entry_info struct passed in by the info parameter.
> + * If the dirname or devid parameters are set to NULL, the dirname and devid
> + * fields in the info parameter will be set to default values. If info already
> + * has a value in the dev fields, we can compare it to the value passed in by
> + * the devid parameter or the default devid to see if we need to open a new
> + * device.
> + */
> +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)
> +    {
> +      devid = grub_env_get ("root");
> +      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;
> +}
> +
> +/*
> + * This function searches for BLS config files based on the data in the info
> + * parameter. If the fallback option is enabled, the default location will be
> + * checked for BLS config files if the first attempt fails.
> + */
> +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;
> +  bool fallback = false;
> +  int r;
> +
> +  do
> +    {
> +      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 == false && 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 = true;
> +	}
> +      else
> +	fallback = false;
> +    }
> +  while (fallback == true);
> +}
> +
> +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 (len >= 5 && grub_strcmp (path + len - 5, ".conf") == 0)

Again, please replace 5 with a constant/sizeof()...

> +	{
> +	  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;
> +  const char *def_entry_end;
> +  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, &def_entry_end, 0);
> +  if (grub_errno != GRUB_ERR_NONE)

This error check is unreliable. Please take a look at commit ac8a37dda
(net/http: Allow use of non-standard TCP/IP ports). It is good example
how it should be done properly.

> +    {
> +      grub_errno = GRUB_ERR_NONE;
> +      return false;
> +    }
> +  if (*def_entry == '\0' || *def_entry_end != '\0')
> +    return false;
> +
> +  if (def_idx == idx)
> +    return true;
> +
> +  return false;
> +}

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel