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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.05.2023 19:00, Andrew Cooper wrote:
> 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.

True. Yet then as a minimal additional change may I ask that you drop
the cast that copy_to_user_hvm() has in exchange for the one(s) you
add?

>>> 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.

I'm pretty sure it would (hence why I didn't suggest so); as said I
think this field was added much later, maybe in the context of the
unified EFI image work.

>>> --- 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.

To be honest - I think I'd be fine with purging (but then better in
a separate patch).

Jan