First of all, next time please add Xen-devel ML to recipients too...
On Wed, Apr 23, 2025 at 09:47:48PM -0500, Aaron Rainbolt wrote:
> Enable GRUB to parse the Xen command line for parameters, and expose
> certain of those parameters to the GRUB config file (or rescue shell)
> as environment variables.
Please make the commit message more verbose, i.e., explain why this
feature it is needed, how it works, etc.
> ---
> grub-core/Makefile.core.def | 2 +
> grub-core/kern/i386/xen/pvh.c | 16 ++
> grub-core/kern/main.c | 12 ++
> grub-core/kern/xen/cmdline.c | 270 ++++++++++++++++++++++++++++++++++
> include/grub/xen.h | 2 +
I think this feature should have a description in the GRUB docs.
> 5 files changed, 302 insertions(+)
> create mode 100644 grub-core/kern/xen/cmdline.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index f70e02e69..79e681a7b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -248,6 +248,7 @@ kernel = {
> xen = term/xen/console.c;
> xen = disk/xen/xendisk.c;
> xen = commands/boot.c;
> + xen = kern/xen/cmdline.c;
>
> i386_xen_pvh = commands/boot.c;
> i386_xen_pvh = disk/xen/xendisk.c;
> @@ -255,6 +256,7 @@ kernel = {
> i386_xen_pvh = kern/i386/xen/tsc.c;
> i386_xen_pvh = kern/i386/xen/pvh.c;
> i386_xen_pvh = kern/xen/init.c;
> + i386_xen_pvh = kern/xen/cmdline.c;
> i386_xen_pvh = term/xen/console.c;
>
> ia64_efi = kern/ia64/efi/startup.S;
> diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
> index 91fbca859..9fb048082 100644
> --- a/grub-core/kern/i386/xen/pvh.c
> +++ b/grub-core/kern/i386/xen/pvh.c
> @@ -352,6 +352,22 @@ grub_xen_setup_pvh (void)
> grub_xen_mm_init_regions ();
>
> grub_rsdp_addr = pvh_start_info->rsdp_paddr;
> + int xen_cmdline_fits = 0;
We have bool type in the GRUB.
> + char *xen_cmdline = (char *) pvh_start_info->cmdline_paddr;
const char?
And please add an empty line here.
> + for (int i = 0; i < 1024; i++)
1024? Where this number come from? And I suggest using constants where
possible.
Additionally, please do not mix code with variable definitions. Define
all variables at the beginning of a function or block.
> + {
> + if (xen_cmdline[i] == '\0')
> + {
> + xen_cmdline_fits = 1;
> + break;
> + }
> + }
> + if (xen_cmdline_fits == 1)
Probably this and xen_cmdline_fits variable is not needed if you merge
the code below with the code above.
> + {
> + grub_strncpy ((char *) grub_xen_start_page_addr->cmd_line,
> + (char *) pvh_start_info->cmdline_paddr,
> + MAX_GUEST_CMDLINE);
> + }
> }
>
> grub_err_t
> diff --git a/grub-core/kern/main.c b/grub-core/kern/main.c
> index 143a232b8..f96b16f73 100644
> --- a/grub-core/kern/main.c
> +++ b/grub-core/kern/main.c
> @@ -40,6 +40,10 @@
> static bool cli_disabled = false;
> static bool cli_need_auth = false;
>
> +#if defined (GRUB_MACHINE_XEN) || defined (GRUB_MACHINE_XEN_PVH)
> +#include <grub/xen.h>
> +#endif
> +
> grub_addr_t
> grub_modules_get_end (void)
> {
> @@ -351,6 +355,14 @@ grub_main (void)
> grub_env_export ("root");
> grub_env_export ("prefix");
>
> + /*
> + * Parse command line parameters from Xen and export them as environment
> + * variables
> + */
> +#if defined (GRUB_MACHINE_XEN) || defined (GRUB_MACHINE_XEN_PVH)
> + grub_parse_xen_cmdline ();
> +#endif
> +
> /* Reclaim space used for modules. */
> reclaim_module_space ();
>
> diff --git a/grub-core/kern/xen/cmdline.c b/grub-core/kern/xen/cmdline.c
> new file mode 100644
> index 000000000..0084f3e77
> --- /dev/null
> +++ b/grub-core/kern/xen/cmdline.c
> @@ -0,0 +1,270 @@
> +/*
> + * 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/env.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +#include <grub/xen.h>
> +
> +#define PARSER_HIT_BACKSLASH 0x1
> +#define PARSER_IN_SINGLE_QUOTES 0x2
> +#define PARSER_IN_DOUBLE_QUOTES 0x4
> +#define PARSER_BASE_WORD_LEN 16
I think it could be enum type.
> +grub_size_t word_list_len;
> +char **word_list;
> +grub_size_t current_word_len;
> +grub_size_t current_word_pos;
> +char *current_word;
> +char current_char;
> +grub_size_t param_dict_len;
> +char **param_keys;
> +char **param_vals;
Probably all of this should be static.
> +static bool
> +append_char_to_word (bool allow_null)
> +{
> + if (!(current_char >= 0x20) || !(current_char <= 0x7E))
I know what you mean but please comment why you accept only limited set
of characters.
> + {
> + if (current_char != '\0' || !allow_null)
allow_null == false please here and in other places too...
> + {
> + return false;
> + }
Please drop unneeded curly brackets...
> + }
> + if (current_word_pos == current_word_len)
> + {
> + current_word_len *= 2;
> + current_word = grub_realloc (current_word, current_word_len);
grub_realloc() may fail...
> + }
> +
> + current_word[current_word_pos] = current_char;
> + current_word_pos++;
> + return true;
> +}
> +
> +static void
> +append_word_to_list (void)
> +{
> + for (grub_size_t i = 0; i < current_word_pos; i++)
Again, please do not mix variable definitions with the code.
> + {
> + if (current_word[i] <= 0x1F || current_word[i] >= 0x7F)
> + {
> + grub_free (current_word);
> + goto reset_word;
> + }
> + }
> + current_char = '\0';
> + if (!append_char_to_word (true))
append_char_to_word (true) == false
> + {
> + grub_error (GRUB_ERR_BUG, N_("couldn't append null terminator to word
> during Xen cmdline parsing"));
Please drop N_() from here...
> + grub_print_error ();
> + grub_exit ();
> + }
> + current_word_len = grub_strlen (current_word) + 1;
> + current_word = grub_realloc (current_word, current_word_len);
Please check for errors when you alloc/realloc memory.
> + word_list_len++;
> + word_list = grub_realloc (word_list, word_list_len * sizeof (char *));
Ditto.
> + word_list[word_list_len - 1] = current_word;
> +
> +reset_word:
Missing space before label.
> + current_word_len = PARSER_BASE_WORD_LEN;
> + current_word_pos = 0;
> + current_word = grub_malloc (current_word_len);
Again, missing error checks are unacceptable.
> +}
> +
> +int
s/int/bool/
> +check_key_is_safe (char *key, grub_size_t len)
> +{
> + /*
> + * Ensure only a-z, A-Z, and _ are allowed.
> + */
> + for (grub_size_t i = 0; i < len; i++)
> + {
> + if (! ((key[i] >= 'A' && key[i] <= 'Z')
> + || (key[i] >= 'a' && key[i] <= 'z')
> + || (key[i] == '_') ) )
This "if" begs for comment.
> + {
> + return 0;
> + }
Redundant curly braces.
> + }
> + return 1;
> +}
> +
> +void
> +grub_parse_xen_cmdline (void)
> +{
> + word_list = grub_malloc (0);
word_list = NULL;
> + current_word_len = PARSER_BASE_WORD_LEN;
> + current_word = grub_malloc (current_word_len);
> + param_keys = grub_malloc (0);
Ditto.
> + param_vals = grub_malloc (0);
Ditto.
And why these variables have to be global?
> + grub_uint8_t parse_flags = 0;
Please use enum type instead of grub_uint8_t.
> + char *cmdline = (char *)grub_xen_start_page_addr->cmd_line;
const char?
And missing space after ")".
> + for (grub_size_t i = 0; i < grub_strlen (cmdline); i++)
Is cmdline always NUL terminated? Should not we impose a (sensible)
command line length limit in the GRUB code?
> + {
> + current_char = cmdline[i];
> +
> + if (parse_flags & PARSER_HIT_BACKSLASH)
> + {
> + parse_flags ^= PARSER_HIT_BACKSLASH;
> + if (!append_char_to_word (false))
> + {
> + goto cleanup;
> + }
Please reduce curly braces to the minimum required...
> + continue;
> + }
> +
> + if (current_char == '\\')
I think parser code should have comments with explanations why we treat
characters in a given way...
> + {
> + parse_flags ^= PARSER_HIT_BACKSLASH;
> + continue;
> + }
> +
> + if (current_char == '\'')
Would not switch/case be better here?
> + {
> + if (parse_flags & PARSER_IN_DOUBLE_QUOTES)
> + {
> + if (!append_char_to_word (false))
> + {
> + goto cleanup;
> + }
> + continue;
> + }
> +
> + parse_flags ^= PARSER_IN_SINGLE_QUOTES;
> + continue;
> + }
> +
> + if (current_char == '"')
Ditto and below/everywhere here...
> + {
> + if (parse_flags & PARSER_IN_SINGLE_QUOTES)
> + {
> + if (!append_char_to_word (false))
> + {
> + goto cleanup;
> + }
> + continue;
> + }
> +
> + parse_flags ^= PARSER_IN_DOUBLE_QUOTES;
> + continue;
> + }
> +
> + if (current_char == ' ')
> + {
> + if (parse_flags & PARSER_IN_SINGLE_QUOTES
> + || parse_flags & PARSER_IN_DOUBLE_QUOTES)
> + {
> + if (!append_char_to_word (false))
> + {
> + goto cleanup;
> + }
> + continue;
> + }
> +
> + append_word_to_list ();
> + continue;
> + }
> +
> + if (!append_char_to_word (false))
> + {
> + goto cleanup;
> + }
> + }
> +
> + if (current_word_pos > 0)
> + {
> + append_word_to_list ();
> + }
> +
> + param_keys = grub_realloc (param_keys, word_list_len * sizeof (char *));
> + param_vals = grub_realloc (param_vals, word_list_len * sizeof (char *));
Again, missing error checks. This kind of code has to be fixed everywhere.
> + for (grub_size_t i = 0; i < word_list_len; i++)
> + {
> + current_word = word_list[i];
> + current_word_len = grub_strlen (current_word) + 1;
> + char *current_word_eq_ptr = grub_strchr (current_word, '=');
Missing empty line.
> + if (current_word_eq_ptr)
> + {
> + grub_size_t eq_idx
> + = (grub_size_t)(current_word_eq_ptr - current_word);
> + grub_size_t pre_eq_len
> + = current_word_len - (current_word_len - eq_idx);
I am OK with lines (a bit) longer than 80 characters.
> + grub_size_t post_eq_len = current_word_len - eq_idx - 1;
> + if (check_key_is_safe (current_word, pre_eq_len))
> + {
> + param_keys[param_dict_len] = grub_malloc (pre_eq_len + 1);
> + param_vals[param_dict_len] = grub_malloc (post_eq_len + 1);
> + grub_strncpy (param_keys[param_dict_len],
> + current_word, pre_eq_len);
> + grub_strncpy (param_vals[param_dict_len],
> + current_word + pre_eq_len + 1, post_eq_len);
> + param_keys[param_dict_len][pre_eq_len] = '\0';
> + param_vals[param_dict_len][post_eq_len] = '\0';
> + param_dict_len++;
> + }
> + }
> + else
> + {
> + if (check_key_is_safe (current_word, current_word_len - 1))
> + {
> + param_keys[param_dict_len] = grub_malloc (current_word_len +
> 1);
> + param_vals[param_dict_len] = grub_malloc (2);
> + grub_strncpy (param_keys[param_dict_len],
> + current_word, current_word_len);
> + param_keys[param_dict_len][current_word_len] = '\0';
> + grub_strcpy (param_vals[param_dict_len], "1\0");
> + param_dict_len++;
> + }
> + }
> + }
> +
> + for (grub_size_t i = 0; i < param_dict_len; i++)
> + {
> + /*
> + * Find keys that start with "xen_grub_env_" and export them
> + * as environment variables.
> + */
> + if (grub_strlen (param_keys[i]) < (sizeof ("xen_grub_env_") - 1))
> + {
> + continue;
> + }
> + if (grub_strncmp (param_keys[i],
> + "xen_grub_env_",
> + sizeof ("xen_grub_env_") - 1) != 0)
> + {
> + continue;
> + }
> + grub_env_set (param_keys[i], param_vals[i]);
> + grub_env_export (param_keys[i]);
> + grub_free (param_keys[i]);
> + grub_free (param_vals[i]);
> + }
> +
> +cleanup:
Missing space before label.
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
