| Message ID | 4998ec735bd7e5a50a229507e2b92ae56ec1ba4b.1708680104.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address some violations of MISRA C Rule 17.7 | expand |
On Fri, 23 Feb 2024, Nicola Vetrini wrote: > These functions never saw a usage of their return value since > they were introduced, so it can be dropped since their usages > violate MISRA C Rule 17.7: > "The value returned by a function having non-void return type shall be used". > > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/drivers/char/consoled.c | 17 +++++------------ > xen/include/xen/consoled.h | 4 ++-- > 2 files changed, 7 insertions(+), 14 deletions(-) > > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c > index 222e01844271..b415b632cecc 100644 > --- a/xen/drivers/char/consoled.c > +++ b/xen/drivers/char/consoled.c > @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void) > static char buf[BUF_SZ + 1]; > > /* Receives characters from a domain's PV console */ > -size_t consoled_guest_rx(void) > +void consoled_guest_rx(void) > { > - size_t recv = 0, idx = 0; > + size_t idx = 0; > XENCONS_RING_IDX cons, prod; > > if ( !cons_ring ) > - return 0; > + return; > > spin_lock(&rx_lock); > > @@ -73,7 +73,6 @@ size_t consoled_guest_rx(void) > char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)]; > > buf[idx++] = c; > - recv++; > > if ( idx >= BUF_SZ ) > { > @@ -92,18 +91,15 @@ size_t consoled_guest_rx(void) > > out: > spin_unlock(&rx_lock); > - > - return recv; > } > > /* Sends a character into a domain's PV console */ > -size_t consoled_guest_tx(char c) > +void consoled_guest_tx(char c) > { > - size_t sent = 0; > XENCONS_RING_IDX cons, prod; > > if ( !cons_ring ) > - return 0; > + return; > > cons = ACCESS_ONCE(cons_ring->in_cons); > prod = cons_ring->in_prod; > @@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c) > goto notify; > > cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c; > - sent++; > > /* Write to the ring before updating the pointer */ > smp_wmb(); > @@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c) > notify: > /* Always notify the guest: prevents receive path from getting stuck. */ > pv_shim_inject_evtchn(pv_console_evtchn()); > - > - return sent; > } > > /* > diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h > index 2b30516b3a0a..bd7ab6329ee8 100644 > --- a/xen/include/xen/consoled.h > +++ b/xen/include/xen/consoled.h > @@ -5,8 +5,8 @@ > > void consoled_set_ring_addr(struct xencons_interface *ring); > struct xencons_interface *consoled_get_ring_addr(void); > -size_t consoled_guest_rx(void); > -size_t consoled_guest_tx(char c); > +void consoled_guest_rx(void); > +void consoled_guest_tx(char c); > > #endif /* __XEN_CONSOLED_H__ */ > /* > -- > 2.34.1 >
On 23.02.2024 23:56, Stefano Stabellini wrote: > On Fri, 23 Feb 2024, Nicola Vetrini wrote: >> These functions never saw a usage of their return value since >> they were introduced, so it can be dropped since their usages >> violate MISRA C Rule 17.7: >> "The value returned by a function having non-void return type shall be used". >> >> No functional change. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> The cleanup is certainly okay, but would one of you mind clarifying in how far this code is relevant for certification? I don't expect there are plans to run shim Xen in any projected production uses for which certification is relevant? (The subject prefix is also unnecessarily wide here, when it's only daemon code which is affected, not console code in general.) Jan
On 2024-02-26 09:00, Jan Beulich wrote: > On 23.02.2024 23:56, Stefano Stabellini wrote: >> On Fri, 23 Feb 2024, Nicola Vetrini wrote: >>> These functions never saw a usage of their return value since >>> they were introduced, so it can be dropped since their usages >>> violate MISRA C Rule 17.7: >>> "The value returned by a function having non-void return type shall >>> be used". >>> >>> No functional change. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > The cleanup is certainly okay, but would one of you mind clarifying in > how > far this code is relevant for certification? I don't expect there are > plans > to run shim Xen in any projected production uses for which > certification is > relevant? (The subject prefix is also unnecessarily wide here, when > it's > only daemon code which is affected, not console code in general.) > I agree on the subject prefix being too wide. The configuration that uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations that may never reach this condition this is relevant, unless its #ifdef is restricted to cases where the call may actually be reachable. consoled_guest_rx was adjusted for uniformity, even though it's technically not included in the configuration under analysis at the moment.
On 26.02.2024 09:23, Nicola Vetrini wrote: > On 2024-02-26 09:00, Jan Beulich wrote: >> On 23.02.2024 23:56, Stefano Stabellini wrote: >>> On Fri, 23 Feb 2024, Nicola Vetrini wrote: >>>> These functions never saw a usage of their return value since >>>> they were introduced, so it can be dropped since their usages >>>> violate MISRA C Rule 17.7: >>>> "The value returned by a function having non-void return type shall >>>> be used". >>>> >>>> No functional change. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> >> The cleanup is certainly okay, but would one of you mind clarifying in >> how >> far this code is relevant for certification? I don't expect there are >> plans >> to run shim Xen in any projected production uses for which >> certification is >> relevant? (The subject prefix is also unnecessarily wide here, when >> it's >> only daemon code which is affected, not console code in general.) >> > > I agree on the subject prefix being too wide. The configuration that > uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations > that may never reach this condition this is relevant, unless its #ifdef > is restricted to cases where the call may actually be reachable. Hmm, I see. There are contradicting goals here then: It being just X86 is to reduce the risk of someone overlooking a build breakage they may introduce. Whereas for certification it's quite the other way around: We'd like to "hide" as much code as possible. Really I would have been inclined to suggest to drop the #ifdef, if possible even without replacing by IS_ENABLED(), but instead leveraging that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n. After all that's a pattern we've been trying to follow. But with your observation is becomes questionable whether extending use of IS_ENABLED() is actually going to be helpful. Stefano - perhaps something to discuss on one of the next meetings? Jan
On Mon, 26 Feb 2024, Jan Beulich wrote: > On 26.02.2024 09:23, Nicola Vetrini wrote: > > On 2024-02-26 09:00, Jan Beulich wrote: > >> On 23.02.2024 23:56, Stefano Stabellini wrote: > >>> On Fri, 23 Feb 2024, Nicola Vetrini wrote: > >>>> These functions never saw a usage of their return value since > >>>> they were introduced, so it can be dropped since their usages > >>>> violate MISRA C Rule 17.7: > >>>> "The value returned by a function having non-void return type shall > >>>> be used". > >>>> > >>>> No functional change. > >>>> > >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>> > >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > >> > >> The cleanup is certainly okay, but would one of you mind clarifying in > >> how > >> far this code is relevant for certification? I don't expect there are > >> plans > >> to run shim Xen in any projected production uses for which > >> certification is > >> relevant? (The subject prefix is also unnecessarily wide here, when > >> it's > >> only daemon code which is affected, not console code in general.) > >> > > > > I agree on the subject prefix being too wide. The configuration that > > uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations > > that may never reach this condition this is relevant, unless its #ifdef > > is restricted to cases where the call may actually be reachable. > > Hmm, I see. There are contradicting goals here then: It being just X86 is > to reduce the risk of someone overlooking a build breakage they may > introduce. Whereas for certification it's quite the other way around: We'd > like to "hide" as much code as possible. > > Really I would have been inclined to suggest to drop the #ifdef, if > possible even without replacing by IS_ENABLED(), but instead leveraging > that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n. This is OK > After all that's a pattern we've been trying to follow. But with your > observation is becomes questionable whether extending use of IS_ENABLED() > is actually going to be helpful. Stefano - perhaps something to discuss > on one of the next meetings? Yes. I checked with the safety manager and his opinion is that IS_ENABLED() is OK to use as a way to disable code from a safety perspective.
On 26.02.2024 23:49, Stefano Stabellini wrote: > On Mon, 26 Feb 2024, Jan Beulich wrote: >> On 26.02.2024 09:23, Nicola Vetrini wrote: >>> On 2024-02-26 09:00, Jan Beulich wrote: >>>> On 23.02.2024 23:56, Stefano Stabellini wrote: >>>>> On Fri, 23 Feb 2024, Nicola Vetrini wrote: >>>>>> These functions never saw a usage of their return value since >>>>>> they were introduced, so it can be dropped since their usages >>>>>> violate MISRA C Rule 17.7: >>>>>> "The value returned by a function having non-void return type shall >>>>>> be used". >>>>>> >>>>>> No functional change. >>>>>> >>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>>> >>>> The cleanup is certainly okay, but would one of you mind clarifying in >>>> how >>>> far this code is relevant for certification? I don't expect there are >>>> plans >>>> to run shim Xen in any projected production uses for which >>>> certification is >>>> relevant? (The subject prefix is also unnecessarily wide here, when >>>> it's >>>> only daemon code which is affected, not console code in general.) >>>> >>> >>> I agree on the subject prefix being too wide. The configuration that >>> uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations >>> that may never reach this condition this is relevant, unless its #ifdef >>> is restricted to cases where the call may actually be reachable. >> >> Hmm, I see. There are contradicting goals here then: It being just X86 is >> to reduce the risk of someone overlooking a build breakage they may >> introduce. Whereas for certification it's quite the other way around: We'd >> like to "hide" as much code as possible. >> >> Really I would have been inclined to suggest to drop the #ifdef, if >> possible even without replacing by IS_ENABLED(), but instead leveraging >> that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n. > > This is OK > > >> After all that's a pattern we've been trying to follow. But with your >> observation is becomes questionable whether extending use of IS_ENABLED() >> is actually going to be helpful. Stefano - perhaps something to discuss >> on one of the next meetings? > > Yes. I checked with the safety manager and his opinion is that > IS_ENABLED() is OK to use as a way to disable code from a safety > perspective. Yet unlike when #ifdef is used, such code would remain visible to e.g. Eclair even after the preprocessing step. Note the context in which I'm bringing this up - if IS_ENABLED() was properly used here (and as tightly as possible), the tool would still have complained, aiui. Jan
On Tue, 27 Feb 2024, Jan Beulich wrote: > On 26.02.2024 23:49, Stefano Stabellini wrote: > > On Mon, 26 Feb 2024, Jan Beulich wrote: > >> On 26.02.2024 09:23, Nicola Vetrini wrote: > >>> On 2024-02-26 09:00, Jan Beulich wrote: > >>>> On 23.02.2024 23:56, Stefano Stabellini wrote: > >>>>> On Fri, 23 Feb 2024, Nicola Vetrini wrote: > >>>>>> These functions never saw a usage of their return value since > >>>>>> they were introduced, so it can be dropped since their usages > >>>>>> violate MISRA C Rule 17.7: > >>>>>> "The value returned by a function having non-void return type shall > >>>>>> be used". > >>>>>> > >>>>>> No functional change. > >>>>>> > >>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >>>>> > >>>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > >>>> > >>>> The cleanup is certainly okay, but would one of you mind clarifying in > >>>> how > >>>> far this code is relevant for certification? I don't expect there are > >>>> plans > >>>> to run shim Xen in any projected production uses for which > >>>> certification is > >>>> relevant? (The subject prefix is also unnecessarily wide here, when > >>>> it's > >>>> only daemon code which is affected, not console code in general.) > >>>> > >>> > >>> I agree on the subject prefix being too wide. The configuration that > >>> uses consoled_guest_tx is #ifdef-ed for x86, so even in configurations > >>> that may never reach this condition this is relevant, unless its #ifdef > >>> is restricted to cases where the call may actually be reachable. > >> > >> Hmm, I see. There are contradicting goals here then: It being just X86 is > >> to reduce the risk of someone overlooking a build breakage they may > >> introduce. Whereas for certification it's quite the other way around: We'd > >> like to "hide" as much code as possible. > >> > >> Really I would have been inclined to suggest to drop the #ifdef, if > >> possible even without replacing by IS_ENABLED(), but instead leveraging > >> that pv_shim ought to be compile-time false whenever CONFIG_PV_SHIM=n. > > > > This is OK > > > > > >> After all that's a pattern we've been trying to follow. But with your > >> observation is becomes questionable whether extending use of IS_ENABLED() > >> is actually going to be helpful. Stefano - perhaps something to discuss > >> on one of the next meetings? > > > > Yes. I checked with the safety manager and his opinion is that > > IS_ENABLED() is OK to use as a way to disable code from a safety > > perspective. > > Yet unlike when #ifdef is used, such code would remain visible to e.g. > Eclair even after the preprocessing step. Note the context in which > I'm bringing this up - if IS_ENABLED() was properly used here (and as > tightly as possible), the tool would still have complained, aiui. Let me check with Roberto about this.
diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c index 222e01844271..b415b632cecc 100644 --- a/xen/drivers/char/consoled.c +++ b/xen/drivers/char/consoled.c @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void) static char buf[BUF_SZ + 1]; /* Receives characters from a domain's PV console */ -size_t consoled_guest_rx(void) +void consoled_guest_rx(void) { - size_t recv = 0, idx = 0; + size_t idx = 0; XENCONS_RING_IDX cons, prod; if ( !cons_ring ) - return 0; + return; spin_lock(&rx_lock); @@ -73,7 +73,6 @@ size_t consoled_guest_rx(void) char c = cons_ring->out[MASK_XENCONS_IDX(cons++, cons_ring->out)]; buf[idx++] = c; - recv++; if ( idx >= BUF_SZ ) { @@ -92,18 +91,15 @@ size_t consoled_guest_rx(void) out: spin_unlock(&rx_lock); - - return recv; } /* Sends a character into a domain's PV console */ -size_t consoled_guest_tx(char c) +void consoled_guest_tx(char c) { - size_t sent = 0; XENCONS_RING_IDX cons, prod; if ( !cons_ring ) - return 0; + return; cons = ACCESS_ONCE(cons_ring->in_cons); prod = cons_ring->in_prod; @@ -121,7 +117,6 @@ size_t consoled_guest_tx(char c) goto notify; cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c; - sent++; /* Write to the ring before updating the pointer */ smp_wmb(); @@ -130,8 +125,6 @@ size_t consoled_guest_tx(char c) notify: /* Always notify the guest: prevents receive path from getting stuck. */ pv_shim_inject_evtchn(pv_console_evtchn()); - - return sent; } /* diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h index 2b30516b3a0a..bd7ab6329ee8 100644 --- a/xen/include/xen/consoled.h +++ b/xen/include/xen/consoled.h @@ -5,8 +5,8 @@ void consoled_set_ring_addr(struct xencons_interface *ring); struct xencons_interface *consoled_get_ring_addr(void); -size_t consoled_guest_rx(void); -size_t consoled_guest_tx(char c); +void consoled_guest_rx(void); +void consoled_guest_tx(char c); #endif /* __XEN_CONSOLED_H__ */ /*
These functions never saw a usage of their return value since they were introduced, so it can be dropped since their usages violate MISRA C Rule 17.7: "The value returned by a function having non-void return type shall be used". No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/drivers/char/consoled.c | 17 +++++------------ xen/include/xen/consoled.h | 4 ++-- 2 files changed, 7 insertions(+), 14 deletions(-)