Re: [XEN PATCH v3] xen/mm: address violations of MISRA C:2012 Rules 8.2 and 8.3

[Federico Serafini <federico.serafini@xxxxxxxxxxx>]

On 20/10/23 08:35, Jan Beulich wrote:
> On 19.10.2023 18:26, Stefano Stabellini wrote:
> > On Thu, 19 Oct 2023, Jan Beulich wrote:
> > > On 19.10.2023 00:43, Stefano Stabellini wrote:
> > > > On Mon, 16 Oct 2023, Jan Beulich wrote:
> > > > > On 03.10.2023 17:24, Federico Serafini wrote:
> > > > > > --- a/xen/arch/x86/mm.c
> > > > > > +++ b/xen/arch/x86/mm.c
> > > > > > @@ -5901,17 +5901,17 @@ int destroy_xen_mappings(unsigned long s, unsigned long 
> > > > > > e)
> > > > > >    * a problem.
> > > > > >    */
> > > > > >   void init_or_livepatch modify_xen_mappings_lite(
> > > > > > -    unsigned long s, unsigned long e, unsigned int _nf)
> > > > > > +    unsigned long s, unsigned long e, unsigned int nf)
> > > > > >   {
> > > > > > -    unsigned long v = s, fm, nf;
> > > > > > +    unsigned long v = s, fm, flags;
> > > > > While it looks correct, I consider this an unacceptably dangerous
> > > > > change: What if by the time this is to be committed some new use of
> > > > > the local "nf" appears, without resulting in fuzz while applying the
> > > > > patch? Imo this needs doing in two steps: First nf -> flags, then
> > > > > _nf -> nf.
> > > > Wouldn't it be sufficient for the committer to pay special attention
> > > > when committing this patch? We are in code freeze anyway, the rate of
> > > > changes affecting staging is low.
> > > Any kind of risk excludes a patch from being a 4.18 candidate, imo.
> > I agree on that. I think it is best to commit it for 4.19 when the tree
> > opens.
> >
> >
> > > That was the case in early RCs already, and is even more so now. Paying
> > > special attention is generally a possibility, yet may I remind you that
> > > committing in general is intended to be a purely mechanical operation?
> > Sure, and I am not asking for a general process change. I am only
> > suggesting that this specific concern on this patch is best solved in
> > the simplest way: by a committer making sure the patch is correct on
> > commit. It is meant to save time for everyone.
> >
> > Jan, if you are OK with it, we could just trust you to commit it the
> > right away as the earliest opportunity.
> If you can get Andrew or Roger to ack this patch in its present shape,
> I won't stand in the way. I'm not going to ack the change without the
> indicated split.
I'll propose a new patch series where changes are splitted as indicated.
I also noticed a discrepancy between Arm and x86 in the name of the
last parameter of xenmem_add_to_physmap_one().
Do you have any suggestions about how to solve it?
If we reach an agreement, then I can put the changes related to the mm 
module in a single patch.
--
Federico Serafini, M.Sc.

Software Engineer, BUGSENG (http://bugseng.com)