| Message ID | 20230724102443.91894-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/dt: Rework the prototype of dt_property_read_string() to help Eclair | expand |
On 24/07/23 12:24, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Eclair vXXX is unable to prove the parameter out_string will only be > used the return of dt_property_read_string() is 0. So it will consider > that MISRA C:2012 Rule 9.1 was violated. This is not correct: ECLAIR cannot prove that the rule is not violated, and hence emits a caution, because the analysis is sound. > > Rework the prototype so the string is returned and use ERR_PTR() to > embed the error code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > > --- > > The XXX should be replaced with the version of Eclair. Nicola, can you > provide it? I don't see as valuable mentioning ECLAIR at all, but rather explain what the change is about (encoding the error value as a return value and removing the **out_value parameter). Regards,
Hi Nicola, On 24/07/2023 13:58, Nicola Vetrini wrote: > > > On 24/07/23 12:24, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Eclair vXXX is unable to prove the parameter out_string will only be >> used the return of dt_property_read_string() is 0. So it will consider >> that MISRA C:2012 Rule 9.1 was violated. > > This is not correct: ECLAIR cannot prove that the rule is not violated, > and hence emits a caution, because the analysis is sound. Ok. How about: "So it will not be able to prove that MISR C:2012 Rule 9.1 wasn't violated"? > >> >> Rework the prototype so the string is returned and use ERR_PTR() to >> embed the error code. >> >> Signed-off-by: Julien Grall <jgrall@amazon.com> >> >> --- >> >> The XXX should be replaced with the version of Eclair. Nicola, can you >> provide it? > > I don't see as valuable mentioning ECLAIR at all, but rather explain > what the change is about (encoding the error value as a return value and > removing the **out_value parameter). If Eclair didn't report a caution, then I would not have spent time writing this patch. Also, the point of mentioning the Eclair version is that if someone ever want to change the prototype back to where it was (e.g. because another tools is unhappy), then we have some details on why it was done and way to reproduce. This would avoid endless argument on the ML on whether it is safe to revert it. So overall, the value is not necessarily for today, but for the future reader. Cheers,
On Mon, 24 Jul 2023, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Eclair vXXX is unable to prove the parameter out_string will only be > used the return of dt_property_read_string() is 0. So it will consider > that MISRA C:2012 Rule 9.1 was violated. > > Rework the prototype so the string is returned and use ERR_PTR() to > embed the error code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> I'll let you sort out the best commit message. For the code changes: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > > The XXX should be replaced with the version of Eclair. Nicola, can you > provide it? > --- > xen/arch/arm/domain_build.c | 4 +++- > xen/arch/arm/psci.c | 10 +++++----- > xen/common/device_tree.c | 15 +++++++-------- > xen/drivers/char/arm-uart.c | 10 ++++++---- > xen/include/xen/device_tree.h | 16 ++++++++-------- > 5 files changed, 29 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 39b4ee03a505..2f98f0b1bd9c 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -3850,7 +3850,9 @@ static int __init construct_domU(struct domain *d, > > kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); > > - rc = dt_property_read_string(node, "xen,enhanced", &dom0less_enhanced); > + dom0less_enhanced = dt_property_read_string(node, "xen,enhanced"); > + > + rc = IS_ERR(dom0less_enhanced) ? PTR_ERR(dom0less_enhanced) : 0; > if ( rc == -EILSEQ || > rc == -ENODATA || > (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c > index 695d2fa1f1b5..8e01b5962c63 100644 > --- a/xen/arch/arm/psci.c > +++ b/xen/arch/arm/psci.c > @@ -8,7 +8,7 @@ > * Copyright (c) 2013 Linaro Limited. > */ > > - > +#include <xen/err.h> > #include <xen/types.h> > #include <xen/init.h> > #include <xen/mm.h> > @@ -85,13 +85,13 @@ static int __init psci_features(uint32_t psci_func_id) > > static int __init psci_is_smc_method(const struct dt_device_node *psci) > { > - int ret; > const char *prop_str; > > - ret = dt_property_read_string(psci, "method", &prop_str); > - if ( ret ) > + prop_str = dt_property_read_string(psci, "method"); > + if ( IS_ERR(prop_str) ) > { > - printk("/psci node does not provide a method (%d)\n", ret); > + printk("/psci node does not provide a method (%ld)\n", > + PTR_ERR(prop_str)); > return -EINVAL; > } > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 0677193ab370..11222c3a8abf 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -191,21 +191,20 @@ bool_t dt_property_read_u64(const struct dt_device_node *np, > > return 1; > } > -int dt_property_read_string(const struct dt_device_node *np, > - const char *propname, const char **out_string) > + > +const char *dt_property_read_string(const struct dt_device_node *np, > + const char *propname) > { > const struct dt_property *pp = dt_find_property(np, propname, NULL); > > if ( !pp ) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > if ( !pp->length ) > - return -ENODATA; > + return ERR_PTR(-ENODATA); > if ( strnlen(pp->value, pp->length) >= pp->length ) > - return -EILSEQ; > - > - *out_string = pp->value; > + return ERR_PTR(-EILSEQ); > > - return 0; > + return pp->value; > } > > /** > diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c > index 8098a968c285..b76d8063beee 100644 > --- a/xen/drivers/char/arm-uart.c > +++ b/xen/drivers/char/arm-uart.c > @@ -21,6 +21,7 @@ > > #include <xen/console.h> > #include <xen/device_tree.h> > +#include <xen/err.h> > #include <xen/param.h> > #include <xen/serial.h> > #include <xen/errno.h> > @@ -55,16 +56,17 @@ static void __init dt_uart_init(void) > { > const char *stdout; > > - ret = dt_property_read_string(chosen, "stdout-path", &stdout); > - if ( ret >= 0 ) > + stdout = dt_property_read_string(chosen, "stdout-path"); > + if ( !IS_ERR(stdout) ) > { > printk("Taking dtuart configuration from /chosen/stdout-path\n"); > if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart)) > >= sizeof(opt_dtuart) ) > printk("WARNING: /chosen/stdout-path too long, truncated\n"); > } > - else if ( ret != -EINVAL /* Not present */ ) > - printk("Failed to read /chosen/stdout-path (%d)\n", ret); > + else if ( PTR_ERR(stdout) != -EINVAL /* Not present */ ) > + printk("Failed to read /chosen/stdout-path (%ld)\n", > + PTR_ERR(stdout)); > } > } > > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index c2eada748915..492204b4feda 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -486,18 +486,18 @@ static inline bool_t dt_property_read_bool(const struct dt_device_node *np, > * if return value if 0. > * > * Search for a property in a device tree node and retrieve a null > - * terminated string value (pointer to data, not a copy). Returns 0 on > - * success, -EINVAL if the property does not exist, -ENODATA if property > - * doest not have value, and -EILSEQ if the string is not > - * null-terminated with the length of the property data. > + * terminated string value (pointer to data, not a copy). Returns a > + * pointer to a null terminated string on success, -EINVAL if the property > + * does not exist, -ENODATA if property doest not have value, and -EILSEQ > + * if the string is not null-terminated with the length of the property data. > + * > + * The caller should use IS_ERR(...) to check if an error is returned. > * > * Note that the empty string "" has length of 1, thus -ENODATA cannot > * be interpreted as an empty string. > - * > - * The out_string pointer is modified only if a valid string can be decoded. > */ > -int dt_property_read_string(const struct dt_device_node *np, > - const char *propname, const char **out_string); > +const char *dt_property_read_string(const struct dt_device_node *np, > + const char *propname); > > /** > * dt_property_match_string() - Find string in a list and return index > -- > 2.40.1 >
Hi Julien, > -----Original Message----- > Subject: [PATCH] xen/dt: Rework the prototype of dt_property_read_string() > to help Eclair > > From: Julien Grall <jgrall@amazon.com> > > Eclair vXXX is unable to prove the parameter out_string will only be > used the return of dt_property_read_string() is 0. So it will consider > that MISRA C:2012 Rule 9.1 was violated. > > Rework the prototype so the string is returned and use ERR_PTR() to > embed the error code. > > Signed-off-by: Julien Grall <jgrall@amazon.com> > --- > The XXX should be replaced with the version of Eclair. Nicola, can you > provide it? I am seeing some ongoing discussions about the commit message, but the code itself looks good to me, and I also tested this patch to make sure things are good, so: Reviewed-by: Henry Wang <Henry.Wang@arm.com> Tested-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 39b4ee03a505..2f98f0b1bd9c 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -3850,7 +3850,9 @@ static int __init construct_domU(struct domain *d, kinfo.vpl011 = dt_property_read_bool(node, "vpl011"); - rc = dt_property_read_string(node, "xen,enhanced", &dom0less_enhanced); + dom0less_enhanced = dt_property_read_string(node, "xen,enhanced"); + + rc = IS_ERR(dom0less_enhanced) ? PTR_ERR(dom0less_enhanced) : 0; if ( rc == -EILSEQ || rc == -ENODATA || (rc == 0 && !strcmp(dom0less_enhanced, "enabled")) ) diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c index 695d2fa1f1b5..8e01b5962c63 100644 --- a/xen/arch/arm/psci.c +++ b/xen/arch/arm/psci.c @@ -8,7 +8,7 @@ * Copyright (c) 2013 Linaro Limited. */ - +#include <xen/err.h> #include <xen/types.h> #include <xen/init.h> #include <xen/mm.h> @@ -85,13 +85,13 @@ static int __init psci_features(uint32_t psci_func_id) static int __init psci_is_smc_method(const struct dt_device_node *psci) { - int ret; const char *prop_str; - ret = dt_property_read_string(psci, "method", &prop_str); - if ( ret ) + prop_str = dt_property_read_string(psci, "method"); + if ( IS_ERR(prop_str) ) { - printk("/psci node does not provide a method (%d)\n", ret); + printk("/psci node does not provide a method (%ld)\n", + PTR_ERR(prop_str)); return -EINVAL; } diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 0677193ab370..11222c3a8abf 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -191,21 +191,20 @@ bool_t dt_property_read_u64(const struct dt_device_node *np, return 1; } -int dt_property_read_string(const struct dt_device_node *np, - const char *propname, const char **out_string) + +const char *dt_property_read_string(const struct dt_device_node *np, + const char *propname) { const struct dt_property *pp = dt_find_property(np, propname, NULL); if ( !pp ) - return -EINVAL; + return ERR_PTR(-EINVAL); if ( !pp->length ) - return -ENODATA; + return ERR_PTR(-ENODATA); if ( strnlen(pp->value, pp->length) >= pp->length ) - return -EILSEQ; - - *out_string = pp->value; + return ERR_PTR(-EILSEQ); - return 0; + return pp->value; } /** diff --git a/xen/drivers/char/arm-uart.c b/xen/drivers/char/arm-uart.c index 8098a968c285..b76d8063beee 100644 --- a/xen/drivers/char/arm-uart.c +++ b/xen/drivers/char/arm-uart.c @@ -21,6 +21,7 @@ #include <xen/console.h> #include <xen/device_tree.h> +#include <xen/err.h> #include <xen/param.h> #include <xen/serial.h> #include <xen/errno.h> @@ -55,16 +56,17 @@ static void __init dt_uart_init(void) { const char *stdout; - ret = dt_property_read_string(chosen, "stdout-path", &stdout); - if ( ret >= 0 ) + stdout = dt_property_read_string(chosen, "stdout-path"); + if ( !IS_ERR(stdout) ) { printk("Taking dtuart configuration from /chosen/stdout-path\n"); if ( strlcpy(opt_dtuart, stdout, sizeof(opt_dtuart)) >= sizeof(opt_dtuart) ) printk("WARNING: /chosen/stdout-path too long, truncated\n"); } - else if ( ret != -EINVAL /* Not present */ ) - printk("Failed to read /chosen/stdout-path (%d)\n", ret); + else if ( PTR_ERR(stdout) != -EINVAL /* Not present */ ) + printk("Failed to read /chosen/stdout-path (%ld)\n", + PTR_ERR(stdout)); } } diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index c2eada748915..492204b4feda 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -486,18 +486,18 @@ static inline bool_t dt_property_read_bool(const struct dt_device_node *np, * if return value if 0. * * Search for a property in a device tree node and retrieve a null - * terminated string value (pointer to data, not a copy). Returns 0 on - * success, -EINVAL if the property does not exist, -ENODATA if property - * doest not have value, and -EILSEQ if the string is not - * null-terminated with the length of the property data. + * terminated string value (pointer to data, not a copy). Returns a + * pointer to a null terminated string on success, -EINVAL if the property + * does not exist, -ENODATA if property doest not have value, and -EILSEQ + * if the string is not null-terminated with the length of the property data. + * + * The caller should use IS_ERR(...) to check if an error is returned. * * Note that the empty string "" has length of 1, thus -ENODATA cannot * be interpreted as an empty string. - * - * The out_string pointer is modified only if a valid string can be decoded. */ -int dt_property_read_string(const struct dt_device_node *np, - const char *propname, const char **out_string); +const char *dt_property_read_string(const struct dt_device_node *np, + const char *propname); /** * dt_property_match_string() - Find string in a list and return index