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
