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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/11/2023 8:40 am, Jan Beulich wrote:
> On 20.11.2023 23:49, 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.
> And the consuming sites confirm this being the case. Hence ...
>
>> Lie to the compiler using a union.
> ... to at least slightly limit the lying, how about ...
>
>> --- 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,
> ... you also switch to using name.cs down below here and ...
>
>> --- 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);
> ... here?
>
> An alternative would be to introduce 'char xen[4] = "xen";' in both
> cases, and use them instead of plain string literals.

They'd have to be static, or dynamically allocated or they'll end up out
of scope, wont they?

I have to admit I find this logic very hard to follow.

Furthermore, I note:

mbi.boot_loader_name = (long)"EFI";

which is exactly the kind of pointer which is liable to end up being
edited via kextra in the other patch.

~Andrew