Re: [PATCH grub-core/kern/xen/init.c] pvgrub2 xen cmdline xenstore var to grubenv

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-10-23 at 17:11 -0700, Mark Pryor wrote:
> When entering the grub2 shell during a pvgrub2 boot, there is no info about the current
> domU in the grubenv (set). Starting with a patch submitted by Olaf Herring I exported
> the xenstore cmdline only.

xenstore? The command line comes from the start info. I'd just drop the
mention of xenstore rather than trying to change it.

> 
> The env var, xen_cmdline, can then be used in the top level script used to make
> the pvgrub2 kernel blob.
> 
> Signed-off-by: Mark Pryor <pryorm09@gmail.com>
> ---
>  grub-core/kern/xen/init.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/grub-core/kern/xen/init.c b/grub-core/kern/xen/init.c
> index 0559c03..2a3112d 100644
> --- a/grub-core/kern/xen/init.c
> +++ b/grub-core/kern/xen/init.c
> @@ -524,6 +524,29 @@ map_all_pages (void)
>    grub_mm_init_region ((void *) heap_start, heap_end - heap_start);
>  }
>  
> +/*
> + * Find all name=val pairs in the provided cmd_line and export them
> + * so that scripts can evaluate the variables for their own purpose.

I don't think that is what this function does, it seems to simply provide a
single env var containing the complete command line. I think that is a fine
behaviour to be starting with.

> + */
> +static void
> +export_cmdline (void)
> +{
> +  char *p;
> +  const char *name="xen_cmdline";
> +
> +  p = grub_malloc (MAX_GUEST_CMDLINE + 1);
> +  if (!p)
> +    return;
> +
> +  grub_memcpy (p, grub_xen_start_page_addr->cmd_line, MAX_GUEST_CMDLINE);
> +  p[MAX_GUEST_CMDLINE] = '\0';
> +
> +  grub_env_set (name, p);
> +  grub_env_export (name);
> +
> +   grub_free (p);

I'm not sure what grub coding style is, but either this last line or all
the others seem to be incorrectly indented (judging from the
inconsistency).

Also, I might be wrong but I think grub_xen_start_page_addr->cmd_line is
guaranteed to be NULL terminated, if you assume that then you can get away
with just calling grub_env_set direct without the copying, I think.

Lastly, if you also cc xen-devel then you might get feedback from people
more knowledgable about such things as whether si->cmd_line is NULL
terminated than I am.

Ian.