| Message ID | 20230714130411.3055187-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/ACPI: Remove the acpi_string type | expand |
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@citrix.com> > --- > CC: George Dunlap <George.Dunlap@eu.citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > > 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 {
On 14/07/2023 2:44 pm, Nicola Vetrini wrote: > > 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/ Fixed, thanks. And the 3rd typo below. > >> 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. Thanks. ~Andrew
On 14.07.2023 15:04, Andrew Cooper wrote: > Typedef-ing a naked pointer like this an anti-pattern which is best avoided. > Furthermore, it's bad to pass a string literate in a mutable type. Delete the > 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. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com>
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 {
Typedef-ing a naked pointer like this an anti-pattern which is best avoided. Furthermore, it's bad to pass a string literate in a mutable type. Delete the 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. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> 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(-)