Re: [RFC 1/4] x86/ioemul: address MISRA C:2012 Rule 9.3

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2023-10-27 23:38, Stefano Stabellini wrote:
> On Thu, 26 Oct 2023, Jan Beulich wrote:
> > On 26.10.2023 14:32, Nicola Vetrini wrote:
> > > On 25/10/2023 09:56, Jan Beulich wrote:
> > >> On 24.10.2023 22:27, Stefano Stabellini wrote:
> > >>> On Tue, 24 Oct 2023, Jan Beulich wrote:
> > >>>> On 24.10.2023 16:31, Nicola Vetrini wrote:
> > >>>>> Partially explicitly initalized .matches arrays result in violations
> > >>>>> of Rule 9.3; this is resolved by using designated initializers,
> > >>>>> which is permitted by the Rule.
> > >>>>>
> > >>>>> Mechanical changes.
> > >>>>>
> > >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > >>>>
> > >>>> While not overly bad, I'm still not really seeing the improvement.
> > >>>> Yet aiui changes induced by Misra are supposed to improve things in
> > >>>> some direction?
> > >>>
> > >>> I think the improvement is clarity, in the sense that the designated
> > >>> initializers make it clearer that the array may be sparsely
> > >>> initialized
> > >>> and that the remaining elements should be initialized to zero
> > >>> automatically.
> > >>
> > >> That's as clear from the original code, imo.
> This specific instance is simple and might be clear either way, but in
> general especially in more complex scenarios and potentially nested
> structures and arrays, it could be harder to figure out and that leads
> to errors. The MISRA checker is a powerful tool to help us make sure 
> the
> code is correct in all cases, but to take advantage of it properly we
> need to get to the point where we don't have violations in the current
> code.
>
> Looking at the results, we have zero violations for Rule 9.3 on ARM
> already and only 55 on x86. It should be possible to fix them all
> mechanically in short order. Of course for that to happen, we need to
> make some compromises. For instance, adding {0} like in the example
> below, or adding [0]=init,[2]=init like in the first version of the
> patch. Taking individually, they might not be all great improvements,
> but all together having the Xen codebase Rule 9.3-free and easy to scan
> for future violations should be.
>
>
>
> > > There's also this functionally equivalent alternative, with or without
> > > the zeros, which
> > > doesn't incur in the risk of mistakenly attempting to initialize the
> > > same element twice,
> > > while also giving an explicit cue to the reader that all elements are
> > > truly zero-initialized.
> > >
> > >           .matches = {
> > >               DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > >               DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> > > +            {0}, {0}
> > >           },
> >
> > Adding a dependency on the array actually having 4 elements (while 
> > iirc
> > we have seen already that we could in principle go down to 3). A 
> > change
> > of this number would then require touching all these sites, which is
> > what we'd like to avoid.
> How often the array needs to change though? Looking at the git history
> it doesn't seem the number of elements ever changed. So I think it is a
> good tradeoff, and I would go with this type of fix (maybe also at the
> other locations mechanically too although I haven't looked at them in
> details).
Hi, any updates on this? Considering the opinions expressed above, what 
would
be the path preferred by the community?

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)