Re: [PATCH] xen/ACPI: Remove the acpi_string type

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 14/07/23 15:04, Andrew Cooper wrote:
> Typedef-ing a naked pointer like this an anti-pattern which is best avoided.
s/this an/this is an/

> Furthermore, it's bad to pass a string literate in a mutable type.  Delete the
s/literate/literal/

> type entirely, and replace it with a plain 'const char *'.
>
> This highlights two futher bugs.  acpi_get_table() already had a mismatch in
> types between it's declaration and definition, and we have declarations for
> acpi_get_handle() and acpi_get_table_header() but no definition at all (nor
> any callers).
>
> This fixes violations of MISRA Rule 7.4:
>
>    A string literal shall not be assigned to an object unless the object's type
>    is "pointer to const-qualified char".
>
> and of Rule 8.3:
>
>    All declarations of an object or function shall use the same names and type
>    qualifiers.
>
> and of Rule 8.6:
>
>    An identifier with external linkage shall have exactly one external
>    definition.
The choice of rules looks good to me, but perhaps Roberto has some 
additional insight on this.
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Roberto Bagnara <roberto.bagnara@xxxxxxxxxxx>
> CC: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>
> Roberto/Nicola: Please double check my choice of rules here, and point out any
> others that I may have missed.
> ---
>   xen/drivers/acpi/tables/tbxface.c |  4 ++--
>   xen/include/acpi/acpixf.h         | 13 ++-----------
>   xen/include/acpi/actypes.h        |  1 -
>   3 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/xen/drivers/acpi/tables/tbxface.c 
> b/xen/drivers/acpi/tables/tbxface.c
> index 21b2e5eae1c7..204d66caea48 100644
> --- a/xen/drivers/acpi/tables/tbxface.c
> +++ b/xen/drivers/acpi/tables/tbxface.c
> @@ -164,7 +164,7 @@ acpi_initialize_tables(struct acpi_table_desc * 
> initial_table_array,
>    *
>    
> *****************************************************************************/
>   acpi_status __init
> -acpi_get_table(char *signature,
> +acpi_get_table(const char *signature,
>                acpi_native_uint instance, struct acpi_table_header **out_table)
>   {
>         acpi_native_uint i;
> @@ -220,7 +220,7 @@ acpi_get_table(char *signature,
>    *
>    
> *****************************************************************************/
>   acpi_status __init
> -acpi_get_table_phys(acpi_string signature, acpi_native_uint instance,
> +acpi_get_table_phys(const char *signature, acpi_native_uint instance,
>                      acpi_physical_address *addr, acpi_native_uint *len)
>   {
>         acpi_native_uint i, j;
> diff --git a/xen/include/acpi/acpixf.h b/xen/include/acpi/acpixf.h
> index ba74908f0478..8b70154b8f96 100644
> --- a/xen/include/acpi/acpixf.h
> +++ b/xen/include/acpi/acpixf.h
> @@ -69,25 +69,16 @@ acpi_status acpi_load_tables(void);
>   acpi_status acpi_load_table(struct acpi_table_header *table_ptr);
>   
>   acpi_status
> -acpi_get_table_header(acpi_string signature,
> -                     acpi_native_uint instance,
> -                     struct acpi_table_header *out_table_header);
> -
> -acpi_status
> -acpi_get_table(acpi_string signature,
> +acpi_get_table(const char *signature,
>                acpi_native_uint instance, struct acpi_table_header **out_table);
>   
>   acpi_status
> -acpi_get_table_phys(acpi_string signature, acpi_native_uint instance,
> +acpi_get_table_phys(const char *signature, acpi_native_uint instance,
>                      acpi_physical_address *addr, acpi_native_uint *len);
>   /*
>    * Namespace and name interfaces
>    */
>   acpi_status
> -acpi_get_handle(acpi_handle parent,
> -               acpi_string pathname, acpi_handle * ret_handle);
> -
> -acpi_status
>   acpi_debug_trace(char *name, u32 debug_level, u32 debug_layer, u32 flags);
>   
>   acpi_status
> diff --git a/xen/include/acpi/actypes.h b/xen/include/acpi/actypes.h
> index f3e95abc3ab3..7023863d0349 100644
> --- a/xen/include/acpi/actypes.h
> +++ b/xen/include/acpi/actypes.h
> @@ -281,7 +281,6 @@ 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 void *acpi_handle;    /* Actually a ptr to a NS Node */
>   
>   struct uint64_struct {
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)