Re: [PATCH] do_multicall and MISRA Rule 8.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2024 15:45, George Dunlap wrote:
> On Fri, Mar 15, 2024 at 2:13 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 15.03.2024 14:55, Julien Grall wrote:
>>> On 15/03/2024 13:24, Jan Beulich wrote:
>>>> On 15.03.2024 13:17, George Dunlap wrote:
>>>>> On Fri, Mar 15, 2024 at 11:57 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>> It sounds like Andy and Stefano feel like this is a situation where "a
>>>>>>> fixed width quantity is meant"; absent any further guidance from the
>>>>>>> CODING_STYLE about when fixed widths should or should not be used, I
>>>>>>> don't think this change would be a violation of CODING_STYLE.
> [snip]
>>>>> Other than "it's in CODING_STYLE", and "it's not really necessary
>>>>> because it's ensured in the assembly code", you haven't advanced a
>>>>> single reason why "uint32_t" is problematic.
>>>>
>>>> And it isn't, I never said it would be. But if we set rules for
>>>> ourselves, why would we take the first opportunity to not respect them?
>>>
>>> I am a bit confused. Reading through the thread you seem to agree that
>>> the written rules are respected here. So what rules are you talking about?
>>
>> What was proposed is use of a fixed width type where according to my
>> reading ./CODING_STYLE says it shouldn't be used.
> 
> This conversation is starting to get frustrating.

Same here.

>  That's simply not
> what it says, and I pointed that out just a few messages ago.
> 
> To reiterate:The text says fixed-width types are OK when a fixed-width
> quantity is "meant"; and that in this case, Stefano and Andy "mean" to
> use a fixed-width quantity.  The implied subtext of that sentence
> could be, "Don't use fixed width types unless there's a good reason to
> use a fixed width", and both Andy and Stefano think there's a good
> reason.  This argument you haven't really addressed at all, except
> with a specious "slippery slope" argument meant to nullify the
> exception; and now you attempt to simply ignore.
> 
> I venture to assert that for most people, the rules are a means to an
> end: That end being code which is correct, robust, fast, easy to
> write, maintain, debug, and review patches for.  What I agreed to,
> when I accepted this patch, was that *in general* we would avoid using
> fixed-width types; but that there were cases where doing so made
> sense.  Some of those were discussed in the thread above.
> 
> Andy and Stefano have already put forward reasons why they think a
> fixed-width type would be better here, which are related to "end
> goals": namely, more robust and easy to maintain code.  When I asked
> what "end goals" would be negatively impacted by using a fixed-width
> type, you explicitly said that there were none!  That the *only*
> reason you're continuing to argue is that we have a document somewhere
> that says we should -- a document which explicitly acknowledges that
> there will be exceptions!

The named reasons simply aren't convincing to me, at all. There's no
loss towards the "end goals" when "unsigned int" is used instead of
"uint32_t". Plus I recall Andrew putting under question use of
"unsigned int" for various hypercall parameter declarations, indicating
his belief that "unsigned long" ought to be used. That's, to me, quite
the opposite of using fixed-width types here, as there's no uniformly
correct fixed-width type to use in that case.

So to me, besides there not having been technical arguments towards
the need to use fixed width types here, there's actually added
confusion about what's actually wanted. Imo if we started using fixed-
width types for hypercall handler parameter declarations, (almost?) all
non-handle ones would likely want to be converted. Consistency, after
all, is part of the "easy to maintain code" goal. Plus without
consistency how would one determine when to use what kind of types.

Bottom line: My take is that here we're discussing a matter of taste
(preferring fixed width types) vs a matter of a supposedly agreed upon
rule (preferring to avoid them). Hence my "there not having been
technical arguments". I therefore view your accuse as simply unfair.

> The ideal response would have been to lay out a reasonably
> comprehensive set of criteria for when to use basic types, when to use
> fixed-width types, and how each criteria advanced the end goals of a
> better codebase.  A sufficient response would have been to lay out
> reasons why "better codebase", in this instance, tilts towards using a
> basic type rather than a fixed-width type.

The main use of fixed width types, to me, is in interface structure
definitions - between Xen and hardware / firmware, or in hypercall
structures. I'm afraid I have a hard time seeing good uses outside of
that. Even in inline assembly, types of variables used for inputs or
outputs don't strictly need to be fixed-width; I can somewhat accept
their use there for documentation purposes.

> Saying "That's what the rules say", without motivating it by
> explaining how it connects to a better codebase, is just not a helpful
> response; and making that argument after it's been pointed out that
> that is *not* what the rules say is even worse.

Well, as above, I view this as unfair as well. The rule's there. It
should be followed. If there really were downsides to following it,
the rule should be amended or dropped. If it was dropped, we'd end
up where we were before: People randomly using fixed width types even
in places we agree they're not suitable for use. And there being no
real handle in reviews to ask for them to change.

In this context, may I remind you that I'm doing a lot more reviews
than you. Having been accused of (and in various cases also guilty of)
bike shedding, it is quite helpful when one can back change requests
by references to (more or less) clearly spelled out rules. If too much
room for interpretation remains (and if there's disagreement about
interpretation), what's written down needs tightening imo. Hence why a
goal of mine has been to get ./CODING_STYLE into less ambiguous shape.
With rather little success, sadly.

Jan