On Thu, May 29, 2025 at 6:59 AM, Avnish Chouhan <avn...@linux.ibm.com> wrote:
> On 2025-05-21 18:21, grub-devel-requ...@gnu.org wrote:
[...]
> > +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");
>
> Hi Alec,
>
> Indentation seems off here!
Hi Avnish,
The reason the indentations looks strange here and through out the patches is
because those are spots where more than 8 spaces were used. In those cases, a
tab is used in the GRUB code. Unfortunately, the formatting of the patch makes
it look strange.
[...]
> > +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;
>
> Indentation seems off!
>
> > +
> > + while (buf != NULL && buf[0] != '\0' && (buf[0] == ' ' ||
> > buf[0] == '\t'))
> > + buf++;
>
> Indentation seems off!
>
> > + if (buf[0] == '#')
> > + {
> > + grub_free (buf);
> > + continue;
> > + }
>
> Indentation seems off in if condition!
>
> > +
> > + separator = grub_strchr (buf, ' ');
> > +
> > + if (separator == NULL)
> > + separator = grub_strchr (buf, '\t');
>
> Indentation seems off!
>
> > +
> > + if (separator == NULL || separator[1] == '\0')
> > + {
> > + grub_free (buf);
> > + break;
> > + }
>
> Indentation seems off in if condition!
>
> > +
> > + separator[0] = '\0';
> > +
> > + do
> > + {
> > + separator++;
> > + }
>
> Indentation seems off!
>
> > + while (*separator == ' ' || *separator == '\t');
>
> What's the use of this while condition? It may result in an infinite
> loop...
>
The purpose of this while loop is to remove any additional spaces or tabs
separating the key from the value. I don't believe this would result in an
infinite loop unless the rest of memory was a space or tab. However, an issue
I do see is we aren't checking if there is a value after removing the excess
spaces and tabs in the loop. I'll fix this.
> > +
> > + err = blsuki_add_keyval (entry, buf, separator);
> > + grub_free (buf);
> > + if (err != GRUB_ERR_NONE)
> > + break;
>
> Indentation seems off!
>
> > + }
> > +
> > + return err;
> > +}
[...]
> > +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;
>
> Indentation seems off!
> Adding one new line would be good here!
Sure thing!
>
> > + 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++;
> > + }
> > + }
>
> Indentation seems off in "if (*value == '$')" and else if conditions.
> Missing else condition in if else ladder. Adding it would be good!
I don't believe an else condition would be necessary since there isn't any code
that needs to be done exclusive of both these conditionals.
>
> > +
> > + end = value;
> > + value++;
> > + }
> > +
> > + if (start != end)
> > + {
> > + buffer = blsuki_field_append (is_env_var, buffer, start, end);
> > + if (buffer == NULL)
> > + return NULL;
>
> Indentation seems off!
>
> > + }
> > +
> > + 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++;
>
> Indentation seems off!
>
> > + if (*separator == '\0')
> > + break;
>
> Indentation seems off!
>
> > +
> > + list = grub_realloc (list, (nlist + 2) * sizeof (char *));
>
> Good to use grub_malloc here!
If list is NULL, grub_realloc() will call grub_malloc(), but I'll take a look
to see if there is a possibility to only call grub_malloc() once before the
loop so we don't keep calling grub_realloc().
>
> > + if (list == NULL)
> > + return NULL;
>
> Indentation seems off!
>
> > +
> > + tmp = grub_strndup (initrd, separator - initrd);
> > + if (tmp == NULL)
> > + {
> > + grub_free (list);
> > + return NULL;
> > + }
>
> Indentation seems off in if condition!
>
> > + 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;
> > +}
[...]
> > +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"));
> > + add_dt_prefix = true;
> > + }
> > +
> > + if (dt_path != NULL)
> > + {
> > + if (add_dt_prefix == true)
> > + {
>
> Indentation seems off!
>
> > + linux_path = blsuki_get_val (entry, "linux", NULL);
> > + slash = grub_strchr (linux_path, '/');
> > + if (slash != NULL)
> > + prefix_len = slash - linux_path + 1;
>
> Adding a new line here would be good!
Sure thing!
>
>
> > + 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) ||
> > + grub_add (size, 1, &size))
>
> Indentation seems off!
>
> > + {
>
> Indentation seems off!
>
> > + grub_error (GRUB_ERR_OUT_OF_RANGE, "overflow detected calculating
> > device tree buffer size");
> > + goto finish;
> > + }
> > +
> > + if (add_dt_prefix == true)
> > + {
>
> Indentation seems off!
>
> > + 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;
>
> Indentation seems off! and anding a new line would be good!
Sure thing!
>
> > + tmp = dt_cmd;
> > + tmp = grub_stpcpy (dt_cmd, "devicetree");
> > + tmp = grub_stpcpy (tmp, " ");
> > + if (add_dt_prefix == true)
> > + tmp = grub_stpcpy (tmp, prefix);
>
> Adding a new line would be good!
>
> > + 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';
> > +
> > + 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);
> > +
> > + 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;
>
> Adding a new line would be good!
Sure thing!
>
> > + argv[0] = title ? title : linux_path;
> > + for (i = 1; i < argc; i++)
> > + argv[i] = args[i-1];
>
> Adding a new line would be good!
Sure thing!
>
> > + 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 : "");
>
> Indentation seems off!
>
> > +
> > + 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);
> > +}
[...]
> > +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)
> > + {
> > + 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;
> > + }
>
> Indentation seems off in if else ladder! adding else condition would be
> good!
Same as before, I don't think there is any code that would require an else
condition here.
>
> > +
> > + 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;
> > +}
[...]
> > diff --git a/grub-core/normal/main.c b/grub-core/normal/main.c
> > index 04d058f55..2d493f21e 100644
> > --- a/grub-core/normal/main.c
> > +++ b/grub-core/normal/main.c
> > @@ -21,6 +21,7 @@
> > #include <grub/net.h>
> > #include <grub/normal.h>
> > #include <grub/dl.h>
> > +#include <grub/menu.h>
> > #include <grub/misc.h>
> > #include <grub/file.h>
> > #include <grub/mm.h>
> > @@ -67,6 +68,11 @@ grub_normal_free_menu (grub_menu_t menu)
> > grub_free (entry->args);
> > }
> >
> > + if (entry->blsuki)
> > + {
> > + entry->blsuki->visible = 0;
> > + }
>
> Brackets not required!
Sure thing! I'll fix this!
>
> Alec , I see many Indentation issues. I have tried pointing out as much
> as I can. Please fix!
> Thank you!
>
> Regards,
> Avnish Chouhan
>
Thanks for taking a look at these patches!
Alec Brown
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
