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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/11/2023 9:02 am, Jan Beulich wrote:
> 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>

Thanks.

> 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.)

We have plenty of examples of this pattern in the codebase already.  Not
quite half of the cases with both static and local variable
declarations, but not far off either.

>From a clarity point of view it is helpful to separate the stack locals
from the globals-with-local-scope.

>
>> @@ -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.)

There's the same pattern just above it, not touched, and this patch is
already getting complicated.

I think there's other cleanup to do here, so I'll defer it to later.

~Andrew