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.coop...@citrix.com>
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
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
