| Message ID | 20231121201540.1528161-4-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: > -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all > this work is useless; it's making a memory allocation just to prepend the > image name which cmdline_cook() intentionally strips back out. > > Simply forgo the work and identify EFI_LOADER as one of the loaders which > doesn't prepend the image name. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one nit: > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, const char *loader_name) > while ( *p == ' ' ) > p++; > > - /* GRUB2 and PVH don't not include image name as first item on command line. */ > - if ( xen_guest || loader_is_grub2(loader_name) ) > + /* > + * PVH, our EFI loader, and GRUB2 don't not include image name as first Just "don't", or "do not"? (I realize it was this way before, but perhaps a good chance to tidy.) Jan > + * item on command line. > + */ > + if ( xen_guest || efi_enabled(EFI_LOADER) || loader_is_grub2(loader_name) ) > return p; > > /* Strip image name plus whitespace. */
On 22/11/2023 9:27 am, Jan Beulich wrote: > On 21.11.2023 21:15, Andrew Cooper wrote: >> -Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all >> this work is useless; it's making a memory allocation just to prepend the >> image name which cmdline_cook() intentionally strips back out. >> >> Simply forgo the work and identify EFI_LOADER as one of the loaders which >> doesn't prepend the image name. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > with one nit: > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, const char *loader_name) >> while ( *p == ' ' ) >> p++; >> >> - /* GRUB2 and PVH don't not include image name as first item on command line. */ >> - if ( xen_guest || loader_is_grub2(loader_name) ) >> + /* >> + * PVH, our EFI loader, and GRUB2 don't not include image name as first > Just "don't", or "do not"? (I realize it was this way before, but perhaps a > good chance to tidy.) Will fix. I completely missed that while rewording it. ~Andrew
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index eebc54180bf7..1a2a2dd83c9b 100644 --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -309,6 +309,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, { union string name; + /* NB place_string() prepends, so call in reverse order. */ if ( cmdline_options ) { name.w = cmdline_options; @@ -317,15 +318,6 @@ static void __init efi_arch_handle_cmdline(CHAR16 *image_name, } if ( cfgfile_options ) place_string(&mbi.cmdline, cfgfile_options); - /* Insert image name last, as it gets prefixed to the other options. */ - if ( image_name ) - { - name.w = image_name; - w2s(&name); - } - else - name.s = "xen"; - place_string(&mbi.cmdline, name.s); if ( mbi.cmdline ) mbi.flags |= MBI_CMDLINE; diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index b171a8f4cf84..200520392481 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -850,8 +850,11 @@ static const char *__init cmdline_cook(const char *p, const char *loader_name) while ( *p == ' ' ) p++; - /* GRUB2 and PVH don't not include image name as first item on command line. */ - if ( xen_guest || loader_is_grub2(loader_name) ) + /* + * PVH, our EFI loader, and GRUB2 don't not include image name as first + * item on command line. + */ + if ( xen_guest || efi_enabled(EFI_LOADER) || loader_is_grub2(loader_name) ) return p; /* Strip image name plus whitespace. */
-Wwrite-strings is unhappy with assigning "xen" to a mutable pointer, but all this work is useless; it's making a memory allocation just to prepend the image name which cmdline_cook() intentionally strips back out. Simply forgo the work and identify EFI_LOADER as one of the loaders which doesn't prepend the image name. 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: * Brand new. I can't find anything which cares about the image name in the slightest. This logic is from bf6501a62e80 ("x86-64: EFI boot code") when EFI was introduced, at whcih point GRUB2 was the only excluded loader in cmdline_cook(). --- xen/arch/x86/efi/efi-boot.h | 10 +--------- xen/arch/x86/setup.c | 7 +++++-- 2 files changed, 6 insertions(+), 11 deletions(-)