| Message ID | 20231121201540.1528161-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Enable -Wwrite-strings | expand |
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@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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
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@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@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
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
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index c41dfdb2bdf8..c0302c6bdd62 100644 --- 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 = { .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0, .max_evtchn_port = -1, @@ -885,7 +887,6 @@ static struct domain *__init create_dom0(const module_t *image, }, }; struct domain *d; - char *cmdline; domid_t domid; if ( opt_dom0_pvh ) @@ -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=") ) { - safe_strcat(dom0_cmdline, " acpi="); - safe_strcat(dom0_cmdline, acpi_param); + safe_strcat(cmdline, " acpi="); + safe_strcat(cmdline, acpi_param); } - - cmdline = dom0_cmdline; } /*
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@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> v2: * New. This unravelled a bit, so I pulled it into a separate patch, but the end result is much clearer to follow IMO. --- xen/arch/x86/setup.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)