[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 <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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |