Re: [PATCH 3/6] xen/efi: Make efi-boot.h compile with -Wwrite-strings

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 20 Nov 2023, Andrew Cooper wrote:
> GCC complains:
> 
>   In file included from arch/arm/efi/boot.c:700:
>   arch/arm/efi/efi-boot.h: In function 'efi_arch_handle_cmdline':
>   arch/arm/efi/efi-boot.h:482:16: error: assignment discards 'const' 
> qualifier from pointer target type [-Werror=discarded-qualifiers]
>     482 |         name.s = "xen";
>         |                ^
> 
> There's no easy option.  .rodata is really read-only, so the fact Xen doesn't
> crash means these strings aren't written to.
> 
> Lie to the compiler using a union.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Roberto Bagnara <roberto.bagnara@xxxxxxxxxxx>
> CC: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> 
> I *really* don't like this, but it's the only suggestion given.

Hi Andrew, I am trying to understand what is the part you don't like. I
understand that union string looks iffy due to being a union with const
and non-const. Is that your concern or you also spotted additional
problems with this?

If that is the only issue, maybe we can come up with a matter in-code
comment or commit message. (The TODO is not immediately obvious to me
what the issue to be improved is.)


> ---
>  xen/arch/arm/efi/efi-boot.h | 2 +-
>  xen/arch/x86/efi/efi-boot.h | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 1c3640bb65fd..c26bf18b68b9 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -479,7 +479,7 @@ static void __init efi_arch_handle_cmdline(CHAR16 
> *image_name,
>          w2s(&name);
>      }
>      else
> -        name.s = "xen";
> +        name.cs = "xen"; /* TODO, find a better way of doing this. */
>  
>      prop_len = 0;
>      prop_len += snprintf(buf + prop_len,
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index eebc54180bf7..e2d256e0517b 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -324,7 +324,8 @@ static void __init efi_arch_handle_cmdline(CHAR16 
> *image_name,
>          w2s(&name);
>      }
>      else
> -        name.s = "xen";
> +        name.cs = "xen"; /* TODO, find a better way of doing this. */
> +
>      place_string(&mbi.cmdline, name.s);
>  
>      if ( mbi.cmdline )
> -- 
> 2.30.2
> 
>