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
