diff mbox series

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

Message ID 76c9f78179a8bb5b4f99b34f163933394f79066c.1698155925.git.nicola.vetrini@bugseng.com (mailing list archive)
State New, archived
Headers show
Series address violations of MISRA C Rule 9.3 | expand

Commit Message

Nicola Vetrini Oct. 24, 2023, 2:31 p.m. UTC
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@bugseng.com>
---
 xen/arch/x86/ioport_emulate.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

--
2.34.1

Comments

Jan Beulich Oct. 24, 2023, 2:48 p.m. UTC | #1
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@bugseng.com>

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?

Jan

> --- a/xen/arch/x86/ioport_emulate.c
> +++ b/xen/arch/x86/ioport_emulate.c
> @@ -44,57 +44,57 @@ static const struct dmi_system_id __initconstrel ioport_quirks_tbl[] = {
>      {
>          .ident = "HP ProLiant DL3xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
>          },
>      },
>      {
>          .ident = "HP ProLiant DL5xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
>          },
>      },
>      {
>          .ident = "HP ProLiant DL7xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
>          },
>      },
>      {
>          .ident = "HP ProLiant ML3xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
>          },
>      },
>      {
>          .ident = "HP ProLiant ML5xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
>          },
>      },
>      {
>          .ident = "HP ProLiant BL2xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
>          },
>      },
>      {
>          .ident = "HP ProLiant BL4xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
>          },
>      },
>      {
>          .ident = "HP ProLiant BL6xx",
>          .matches = {
> -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
>          },
>      },
>      { }
> --
> 2.34.1
Stefano Stabellini Oct. 24, 2023, 8:27 p.m. UTC | #2
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@bugseng.com>
> 
> 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.
 

> > --- a/xen/arch/x86/ioport_emulate.c
> > +++ b/xen/arch/x86/ioport_emulate.c
> > @@ -44,57 +44,57 @@ static const struct dmi_system_id __initconstrel ioport_quirks_tbl[] = {
> >      {
> >          .ident = "HP ProLiant DL3xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant DL5xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant DL7xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant ML3xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant ML5xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant BL2xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant BL4xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
> >          },
> >      },
> >      {
> >          .ident = "HP ProLiant BL6xx",
> >          .matches = {
> > -            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > -            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> > +            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
> > +            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
> >          },
> >      },
> >      { }
> > --
> > 2.34.1
>
Jan Beulich Oct. 25, 2023, 7:56 a.m. UTC | #3
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@bugseng.com>
>>
>> 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.

Jan
Nicola Vetrini Oct. 26, 2023, 12:32 p.m. UTC | #4
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@bugseng.com>
>>> 
>>> 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.
> 
> Jan

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}
          },
Jan Beulich Oct. 26, 2023, 12:39 p.m. UTC | #5
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@bugseng.com>
>>>>
>>>> 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.
> 
> 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.

Jan
Stefano Stabellini Oct. 27, 2023, 9:38 p.m. UTC | #6
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@bugseng.com>
> >>>>
> >>>> 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).
Nicola Vetrini Nov. 6, 2023, 2:20 p.m. UTC | #7
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@bugseng.com>
>> >>>>
>> >>>> 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?
Stefano Stabellini Nov. 11, 2023, 1:23 a.m. UTC | #8
On Mon, 6 Nov 2023, Nicola Vetrini wrote:
> > > > 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?

Hi Jan, to bring this discussion to a conclusion, I think we have these
options:

1) fix these violations by adding {}, {}
2) fix these violations by adding [0]=xxx,[1]=xxx
3) deviate these violations by adding /* SAF-safe-xxx */
4) remove the MISRA rule 9.3 from docs/misra/rules.rst

Let's make a decision. My preference is 1) as we only have ~50
violations.
Jan Beulich Nov. 13, 2023, 7:49 a.m. UTC | #9
On 11.11.2023 02:23, Stefano Stabellini wrote:
> On Mon, 6 Nov 2023, Nicola Vetrini wrote:
>>>>> 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?
> 
> Hi Jan, to bring this discussion to a conclusion, I think we have these
> options:
> 
> 1) fix these violations by adding {}, {}
> 2) fix these violations by adding [0]=xxx,[1]=xxx
> 3) deviate these violations by adding /* SAF-safe-xxx */
> 4) remove the MISRA rule 9.3 from docs/misra/rules.rst
> 
> Let's make a decision. My preference is 1) as we only have ~50
> violations.

Of these, to be honest, my preference would be 4. Just that that's
undesirable for other reasons. But have we thought of alternatives, say
a variadic macro that would supply the "missing" initializers? Imo such
decisions shouldn't be rushed; there are enough other issues to take
care of in the meantime. A sound solution is, I think, generally
preferable to a quick one. (Whether my new suggestion is "sound" I of
course can't tell, until it was tried out and the overall result /
effects can be inspected.)

Jan
Stefano Stabellini Nov. 16, 2023, 12:10 a.m. UTC | #10
On Mon, 13 Nov 2023, Jan Beulich wrote:
> On 11.11.2023 02:23, Stefano Stabellini wrote:
> > On Mon, 6 Nov 2023, Nicola Vetrini wrote:
> >>>>> 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?
> > 
> > Hi Jan, to bring this discussion to a conclusion, I think we have these
> > options:
> > 
> > 1) fix these violations by adding {}, {}
> > 2) fix these violations by adding [0]=xxx,[1]=xxx
> > 3) deviate these violations by adding /* SAF-safe-xxx */
> > 4) remove the MISRA rule 9.3 from docs/misra/rules.rst
> > 
> > Let's make a decision. My preference is 1) as we only have ~50
> > violations.
> 
> Of these, to be honest, my preference would be 4. Just that that's
> undesirable for other reasons. But have we thought of alternatives, say
> a variadic macro that would supply the "missing" initializers? Imo such
> decisions shouldn't be rushed; there are enough other issues to take
> care of in the meantime. A sound solution is, I think, generally
> preferable to a quick one. (Whether my new suggestion is "sound" I of
> course can't tell, until it was tried out and the overall result /
> effects can be inspected.)

I don't like the idea of the variadic macro as we should attempt to make
things more obviously correct, rather than more obscure.

Thinking out of the box, what if we added a single {} E.g.:

        .ident = "HP ProLiant DL3xx",
        .matches = {
            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
            {}
        },

It would accomplish the goal of highlighting that there are more members
of the array that gets initialized to zero. At the same time it wouldn't
require the introductino of [0] and [1] that as we have seen are error
prone and it wouldn't depend on the exact number of elements like adding
one {} per missing initialization. To be clear, I am suggesting adding a
single {} only.

Nicola, what do you think? Would it be OK for MISRA / ECLAIR?
diff mbox series

Patch

diff --git a/xen/arch/x86/ioport_emulate.c b/xen/arch/x86/ioport_emulate.c
index 6caeb3d470ce..4f8d5136746d 100644
--- a/xen/arch/x86/ioport_emulate.c
+++ b/xen/arch/x86/ioport_emulate.c
@@ -44,57 +44,57 @@  static const struct dmi_system_id __initconstrel ioport_quirks_tbl[] = {
     {
         .ident = "HP ProLiant DL3xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL3"),
         },
     },
     {
         .ident = "HP ProLiant DL5xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL5"),
         },
     },
     {
         .ident = "HP ProLiant DL7xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant DL7"),
         },
     },
     {
         .ident = "HP ProLiant ML3xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML3"),
         },
     },
     {
         .ident = "HP ProLiant ML5xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant ML5"),
         },
     },
     {
         .ident = "HP ProLiant BL2xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL2"),
         },
     },
     {
         .ident = "HP ProLiant BL4xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL4"),
         },
     },
     {
         .ident = "HP ProLiant BL6xx",
         .matches = {
-            DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
-            DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
+            [0] = DMI_MATCH(DMI_BIOS_VENDOR, "HP"),
+            [1] = DMI_MATCH(DMI_PRODUCT_NAME, "ProLiant BL6"),
         },
     },
     { }