Re: [PATCH RFC] xen: Enable -Wwrite-strings

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/05/2023 11:34 am, Jan Beulich wrote:
> On 16.05.2023 22:34, Andrew Cooper wrote:
>> Following on from the MISRA discussions.
>>
>> On x86, most are trivial.  The two slightly suspect cases are __hvm_copy()
>> where constness is dependent on flags,
> But do we ever pass string literals into there? I certainly would
> like to avoid the explicit casts to get rid of the const there.

The thing which trips it up is the constness of the cmdline param in the
construct_dom0() calltree.  It may have been tied up in the constness
from cmdline_cook() - I wasn't paying that much attention.

Irrespective, from a conceptual point of view, we ought to be able to
use the copy_to_* helpers from a const source.

>> and kextra in __start_xen() which only
>> compiles because of laundering the pointer through strstr().
> The sole string literal there looks to be the empty string in
> cmdline_cook(), which could be easily replaced, I think:
>
> static char * __init cmdline_cook(char *p, const char *loader_name)
> {
>     static char __initdata empty[] = "";
>
>     p = p ? : empty;
>
> Yet of course only if we were unhappy with the strstr() side effect.

It's quite possible we can do something better here.  This logic looks
unnecessarily complicated and fragile.

>
>> The one case which I can't figure out how to fix is EFI:
>>
>>   In file included from arch/x86/efi/boot.c:700:
>>   arch/x86/efi/efi-boot.h: In function ‘efi_arch_handle_cmdline’:
>>   arch/x86/efi/efi-boot.h:327:16: error: assignment discards ‘const’ 
>> qualifier from pointer target type [-Werror=discarded-qualifiers]
>>     327 |         name.s = "xen";
>>         |                ^
>>   cc1: all warnings being treated as errors
>>
>> Why do we have something that looks like this ?
>>
>>   union string {
>>       CHAR16 *w;
>>       char *s;
>>       const char *cs;
>>   };
> Because that was the least clutter (at respective use sites) that I
> could think of at the time. Looks like you could simply assign to
> name.cs, now that we have that field (iirc it wasn't there originally).
> Of course that's then only papering over the issue.

Well yes.  If it's only this one, we could use the same initconst trick
and delete the cs field, but I suspect the fields existence means it
would cause problems elsewhere.

>
>> --- a/xen/include/acpi/actypes.h
>> +++ b/xen/include/acpi/actypes.h
>> @@ -281,7 +281,7 @@ typedef acpi_native_uint acpi_size;
>>   */
>>  typedef u32 acpi_status;    /* All ACPI Exceptions */
>>  typedef u32 acpi_name;              /* 4-byte ACPI name */
>> -typedef char *acpi_string;  /* Null terminated ASCII string */
>> +typedef const char *acpi_string;    /* Null terminated ASCII string */
>>  typedef void *acpi_handle;  /* Actually a ptr to a NS Node */
> For all present uses that we have this change looks okay, but changing
> this header leaves me a little uneasy. At the same time I have no
> better suggestion.

I was honestly tempted to purge this typedef with prejudice.  Hiding
indirection like this is nothing but an obfuscation technique.

~Andrew