Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.11.2023 21:15, Andrew Cooper wrote:
> There's a confusing mix of variables; a static dom0_cmdline[], and a cmdline
> pointer which points to image->string before being pointed at the static
> buffer in order to be passed into construct_dom0().  cmdline being a mutable
> pointer falls over -Wwrite-strings builds.
> 
> Delete the cmdline pointer, and rename dom0_cmdline[] to cmdline extending it
> to have full function scope.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with two remarks:

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -873,6 +873,8 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>                                           module_t *initrd, const char 
> *kextra,
>                                           const char *loader)
>  {
> +    static char __initdata cmdline[MAX_GUEST_CMDLINE];
> +
>      struct xen_domctl_createdomain dom0_cfg = {

While I think I see why you're adding the blank line there, I'm not overly
happy about you doing so: Our usual use of blank lines after declarations
is to separate from statements, not from other (in this case non-static)
declarations. (And really, just a remark, leaving it to you to keep as is
or adjust.)

> @@ -913,33 +914,30 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>          panic("Error creating d%uv0\n", domid);
>  
>      /* Grab the DOM0 command line. */
> -    cmdline = image->string ? __va(image->string) : NULL;
> -    if ( cmdline || kextra )
> +    if ( image->string || kextra )
>      {
> -        static char __initdata dom0_cmdline[MAX_GUEST_CMDLINE];
> -
> -        cmdline = cmdline_cook(cmdline, loader);
> -        safe_strcpy(dom0_cmdline, cmdline);
> +        if ( image->string )
> +            safe_strcpy(cmdline, cmdline_cook(__va(image->string), loader));
>  
>          if ( kextra )
>              /* kextra always includes exactly one leading space. */
> -            safe_strcat(dom0_cmdline, kextra);
> +            safe_strcat(cmdline, kextra);
>  
>          /* Append any extra parameters. */
> -        if ( skip_ioapic_setup && !strstr(dom0_cmdline, "noapic") )
> -            safe_strcat(dom0_cmdline, " noapic");
> +        if ( skip_ioapic_setup && !strstr(cmdline, "noapic") )
> +            safe_strcat(cmdline, " noapic");
> +
>          if ( (strlen(acpi_param) == 0) && acpi_disabled )
>          {
>              printk("ACPI is disabled, notifying Domain 0 (acpi=off)\n");
>              safe_strcpy(acpi_param, "off");
>          }
> -        if ( (strlen(acpi_param) != 0) && !strstr(dom0_cmdline, "acpi=") )
> +
> +        if ( (strlen(acpi_param) != 0) && !strstr(cmdline, "acpi=") )

As you're touching this anyway, how about replacing the strlen() by just
*acpi_param? We don't really care exactly how long the string is. (As an
aside, strstr() uses like the one here are of course also pretty fragile.
But of course that's nothing to care about in this change.)

Jan