On Wed, 28 May 2025 17:12:19 +0200 Daniel Kiper <dki...@net-space.pl> wrote:
> First of all, next time please add Xen-devel ML to recipients too...
Apologies, didn't realize that list would be appropriate. I will add
them in the next version of the patch.
> 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.
1024 is the maximum Xen command line length specified by
include/xen/xen.h. I think I can just use the MAX_GUEST_CMDLINE define
set by that file here, is that acceptable?
> Additionally, please do not mix code with variable definitions. Define
> all variables at the beginning of a function or block.
Will do, but this should probably be added to the coding style
documentation at some point in the future. (It looks like a lot of the
issues with this patch are coding style issues that I could have
avoided had the details been documented.)
> > + {
> > + 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...
+1, also might be useful to document.
> > + {
> > + return false;
> > + }
>
> Please drop unneeded curly brackets...
+1, also might be useful to document.
> > + }
> > + if (current_word_pos == current_word_len)
> > + {
> > + current_word_len *= 2;
> > + current_word = grub_realloc (current_word,
> > current_word_len);
>
> grub_realloc() may fail...
I'm embarrassed I forgot to error-check every single memory allocation,
I'm too used to languages that immediately crash the program if memory
runs out and forget that C will just keep moving forward with a failed
allocation if you don't handle it yourself. Thanks for your patience,
I'll make sure all these are resolved in the next iteration of the
patch.
> > + }
> > +
> > + 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...
Will do, when I wrote this I thought this message needed to be
translatable, but in retrospect this should be unreachable code
(barring an allocation failure), so if it ever is reached, having an
exact string that can be grepped for easily will be valuable.
> > + 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.
Also would be good to document as a coding style requirement.
> > + 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.
The comment is above the "for" loop one line up. I can move it inside
the loop if that's preferable.
> > + {
> > + return 0;
> > + }
>
> Redundant curly braces.
>
> > + }
> > + return 1;
> > +}
> > +
> > +void
> > +grub_parse_xen_cmdline (void)
> > +{
> > + word_list = grub_malloc (0);
>
> word_list = NULL;
I was going off of the documentation in `man 3 malloc`, which states
"if size is 0, then malloc() returns a unique pointer value that can
later be successfully passed to free()`. Looking closer at the memory
management code, I see `grub_free()` can handle a null pointer, but
wasn't aware of that when writing this, and was trying to avoid a
memory management error since this and the other `grub_malloc (0)`'d
variables are unconditionally `grub_free`'d in the cleanup section.
> > + 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?
It looks like they don't have to be, I thought it was clearer to have
them declared in the same place as the other command line parsing state
variables though. I'll move them into the function where they are used.
> > + 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 ")".
Huh, looks like clang-format didn't catch that one for me... or I
forgot to re-run it before submission, I don't remember now. In any
event, will fix.
> > + 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?
If GRUB is started in a PVH VM, `grub_xen_setup_pvh` will check the
command line to ensure it is NUL-terminated before loading it into the
`grub_xen_start_page_addr->cmd_line`. In a PV VM, this struct is
populated by Xen itself and GRUB simply grabs a pointer to it that
Xen passes it, so I guess in theory a Xen bug could result in this
command line not being NUL-terminated. That would be a pretty severe
bug on Xen's part, but I suppose it would be good to double-check just
in case.
> > + {
> > + 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?
Yes, looking at it now I agree it would be.
> > + {
> > + 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
pgpVEEXbGnGHz.pgp
Description: OpenPGP digital signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
