Re: [XEN PATCH 1/2] xen/console: drop return value from consoled_guest_rx/tx

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxxx>
> >>>>>
> >>>>> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >>>>
> >>>> 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.