Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()
On 22/11/2023 4:20 pm, Andrew Cooper wrote:
> On 22/11/2023 9:02 am, Jan Beulich wrote:
>> On 21.11.2023 21:15, Andrew Cooper wrote:
>>> @@ -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.
It turns out that the optimiser already makes this transformation.
I shan't admit to how long I've just spent thinking I had a problem with
my build...
~Andrew
Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()
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
> Reviewed-by: Jan Beulich
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
Re: [PATCH v2 1/5] x86/setup: Clean up cmdline handling in create_dom0()
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
Reviewed-by: Jan Beulich
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
