Re: [XEN PATCH 4/5] x86/x86_emulate: change parameter name from 's' to 'state'

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2023 00:05, Stefano Stabellini wrote:
> On Thu, 6 Jul 2023, Jan Beulich wrote:
>> On 06.07.2023 00:49, Stefano Stabellini wrote:
>>> On Tue, 4 Jul 2023, Jan Beulich wrote:
>>>> On 29.06.2023 21:31, Stefano Stabellini wrote:
>>>>> On Thu, 29 Jun 2023, Federico Serafini wrote:
>>>>>> Change parameter name from 's' to 'state' in function definitions in
>>>>>> order to:
>>>>>> 1) keep consistency with the parameter names used in the corresponding
>>>>>>    declarations;
>>>>>> 2) keep consistency with parameter names used within x86_emulate.h;
>>>>>> 3) fix violations of MISRA C:2012 Rule 8.3.
>>>>>>
>>>>>> Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
>>>>>
>>>>> You could use x86emul: as tag in the title. I'll let Jan choose the tag
>>>>> he prefers.
>>>>
>>>> x86emul: or x86/emul: is what we commonly use. That said, I don't like
>>>> this change. The files touched are pretty new, and it was deliberate
>>>> that I used s, not state, for the names. This is shorthand much like
>>>> (globally) we use v (instead of vcpu) and d (instead of domain).
>>>
>>> Are you suggesting that the functions changed in this patch should be
>>> adapted in the other direction instead?  Meaning that the declaration is
>>> changed to match the definition instead of the opposite?
>>>
>>> If so, are you referring to all the functions changed in this patch? Or
>>> only some?
>>
>> All of the files touched here are ones which were recently introduced,
>> and which are deliberately the way they are. This "deliberately" really
>> goes as far as declarations and definitions disagreeing in names: For
>> the former, what matters are adjacent declarations in the header. For
>> the latter what matters is code readability. I'm sorry, I think the
>> Misra rule simply gets in the way of the original intentions here (and
>> I continue to disagree with there being any confusion from name
>> mismatches between declarations and definitions, the more that in the
>> case here it would be easy to avoid by simply omitting names in
>> declarations, but that is violating yet another rule I don't fully
>> agree with either, as voiced when discussing it).
>>
>> My preferred course of action here would be to simply drop the patch.
>> The least bad adjustment, if one is absolutely necessary, would be to
>> change the declarations, but then in a way that all adjacent ones
>> remain consistent (which may in turn require some _other_ definitions
>> to change). The mid- to long-term goal certainly is to use "s" more
>> where "state" may be used right now.
> 
> 
> If we drop this patch then we have the problem that Eclair and other
> scanners will detect these as violations. Also this source file would
> not be consistent with the rest of the codebase causing issues in the
> future. Any patch updating this file would trigger new MISRA C
> violations in the scanners.
> 
> So I don't think we can drop this patch but we could add deviations. I
> think your suggestion of "changing the declaration in a way that all
> adjacent ones remain consistent" is better, but let me also provide this
> option.
> 
> Basically, we would keep these declarations/definitions as is, and add
> a one-line in-code comment for each deviation. Such as:
> 
> /* SAF-2-safe R8.3 */
> bool cf_check
> x86_insn_is_mem_write(const struct x86_emulate_state *state,
>                       const struct x86_emulate_ctxt *ctxt);
> 
> Your suggestion of changing the declaration is better in my opinion. Do
> you agree?

Yes. I'm not really happy with any of the options resulting from us following
the various involved rules (also in particular 8.2), but this looks to be the
least bad of them.

Jan