Re: [PATCH 2/6] x86/setup: Rework cmdline_cook() to be compatible with -Wwrite-strings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2023 23:49, Andrew Cooper wrote:
> Constify both cmdline variables in create_dom0() and __start_xen().
> Initialise Xen's variable to the empty string to simplify the parsing logic.
> 
> Update cmdline_cook() to take and return const pointers, changing it to have
> an early exit for a NULL input (which can happen if the mbi-> pointers happen
> to be 0).
> 
> Note this only compiles because strstr() launders the const off the pointer
> when assigning to the mutable kextra, but that logic only mutates the
> mbi->cmdline buffer.

And a good static analyzer would spot this. At the very least I think this
warrants a comment next to that code. But really I'm inclined to re-write
this to eliminate the issue altogether; I'll try to remember to do so once
your change has gone in.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -837,9 +837,10 @@ static bool __init loader_is_grub2(const char 
> *loader_name)
>      return (p != NULL) && (p[5] != '0');
>  }
>  
> -static char * __init cmdline_cook(char *p, const char *loader_name)
> +static const char *__init cmdline_cook(const char *p, const char 
> *loader_name)
>  {
> -    p = p ? : "";
> +    if ( !p )
> +        return "";

This change is now needed only for create_dom0(), whereas the call site
change to __start_xen() is redundant with the change here. Did you
consider doing a similar transformation in create_dom0(), thus
eliminating the need for this check altogether? Alternatively I'd like
to ask that ...

> @@ -885,7 +886,7 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>          },
>      };
>      struct domain *d;
> -    char *cmdline;
> +    const char *cmdline;
>      domid_t domid;
>  
>      if ( opt_dom0_pvh )
> @@ -971,8 +972,8 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>  /* SAF-1-safe */
>  void __init noreturn __start_xen(unsigned long mbi_p)
>  {
> -    const char *memmap_type = NULL, *loader;
> -    char *cmdline, *kextra;
> +    const char *memmap_type = NULL, *loader, *cmdline = "";
> +    char *kextra;
>      void *bsp_stack;
>      struct cpu_info *info = get_cpu_info(), *bsp_info;
>      unsigned int initrdidx, num_parked = 0;
> @@ -1027,9 +1028,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                                             : "unknown";
>  
>      /* Parse the command-line options. */
> -    cmdline = cmdline_cook((mbi->flags & MBI_CMDLINE) ?
> -                           __va(mbi->cmdline) : NULL,
> -                           loader);
> +    if ( mbi->flags & MBI_CMDLINE )
> +        cmdline = cmdline_cook(__va(mbi->cmdline), loader);
> +
>      if ( (kextra = strstr(cmdline, " -- ")) != NULL )
>      {
>          /*

... this last hunk be dropped, along with cmdline's initializer. No need
for extra code churn when not gaining us anything. (Also but not only
because the reformatting here is actually beneficial from a readability
pov imo, the variant with applying the same transformation to create_dom0()
would seem preferable to me.)

Jan