Re: [XEN PATCH v2] x86: make function declarations consistent with definitions

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 4 Jul 2023, Jan Beulich wrote:
> On 04.07.2023 12:23, Federico Serafini wrote:
> > Change mechanically the parameter names and types of function
> > declarations to be consistent with the ones used in the corresponding
> > definitions so as to fix violations of MISRA C:2012 Rule 8.3 ("All
> > declarations of an object or function shall use the same names and type
> > qualifiers") and MISRA C:2012 Rule 8.2 ("Function types shall be in
> > prototype form with named parameters").
> > 
> > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> 
> On top of my earlier remark (when this was part of a series):

Hi Jan,

I am not addressing specifically this comment. I am trying to build a
common understanding on how to do things so that we can go faster in the
future.

In general, as discussed at Xen Summit, in order to successfully merge
large numbers of changes in the coming weeks we should try to keep
mechanical changes mechanical. Separate non-mechanical changes into
different patches.

This patch is large but mechanical. If I understand you correctly, you
are asking:
1) to split the patch into smaller patches
2) make a couple of non-mechanical changes described below


For 1), in my opinion it is not necessary as long as all changes remain
mechanical. If some changes are not mechanical they should be split out.
So if you are asking non-mechanical changes in 2), then 2) should be
split out but everything else could stay in the same patch.

If you'd still like the patch to be split, OK but then you might want to
suggest exactly how it should be split because it is not obvious: all
changes are similar, local, and mechanical. I for one wouldn't know how
you would like this patch to be split.


For 2), I would encourage you to consider the advantage of keeping the
changes as-is in this patch, then send out a patch on top the way you
prefer. That is because it costs you more time to describe how you
would like these lines to be changed in English and review the full
patch a second time, than change them yourself and anyone could ack them
(feel free to CC me).

For clarity: I think it is totally fine that you have better suggestions
on parameter names. I am only pointing out that providing those
suggestions as feedback in an email reply is not a very efficient way to
get it done.


> > --- a/xen/arch/x86/cpu/mcheck/x86_mca.h
> > +++ b/xen/arch/x86/cpu/mcheck/x86_mca.h
> > @@ -113,7 +113,7 @@ static inline int mcabanks_test(int bit, struct 
> > mca_banks* banks)
> >      return test_bit(bit, banks->bank_map);
> >  }
> >  
> > -struct mca_banks *mcabanks_alloc(unsigned int nr);
> > +struct mca_banks *mcabanks_alloc(unsigned int nr_mce_banks);
> 
> I'm not convinced here.
> 
> > --- a/xen/arch/x86/hvm/rtc.c
> > +++ b/xen/arch/x86/hvm/rtc.c
> > @@ -59,7 +59,7 @@ enum rtc_mode {
> >  static void rtc_copy_date(RTCState *s);
> >  static void rtc_set_time(RTCState *s);
> >  static inline int from_bcd(RTCState *s, int a);
> > -static inline int convert_hour(RTCState *s, int hour);
> > +static inline int convert_hour(RTCState *s, int raw);
> 
> Nor here.
> 
> > --- a/xen/arch/x86/include/asm/guest_pt.h
> > +++ b/xen/arch/x86/include/asm/guest_pt.h
> > @@ -422,7 +422,7 @@ static inline unsigned int 
> > guest_walk_to_page_order(const walk_t *gw)
> >  
> >  bool
> >  guest_walk_tables(const struct vcpu *v, struct p2m_domain *p2m,
> > -                  unsigned long va, walk_t *gw, uint32_t pfec,
> > +                  unsigned long va, walk_t *gw, uint32_t walk,
> >                    gfn_t top_gfn, mfn_t top_mfn, void *top_map);
> 
> While the definition's use of "walk" makes clear why the variable is
> named the way it is despite being used with PFEC_* constants, not
> naming it "pfec" here will add confusion, as the connection to those
> constants will be lost. One will then be forced to go look at the
> definition, when looking at the declaration ought to be sufficient.
> 
> I'm not going to look further, but instead repeat my suggestion to
> split this patch. Besides reducing the Cc list(s), that'll also
> help getting in parts which are uncontroversial (like e.g. the
> change to xen/arch/x86/hvm/vioapic.c).