Re: [XEN PATCH] xen: fixed violations of MISRA C:2012 Rule 3.1

[nicola <nicola.vetrini@xxxxxxxxxxx>]

On 13/06/23 10:27, Jan Beulich wrote:

> On 13.06.2023 09:42, Nicola Vetrini wrote:
> > --- a/xen/common/xmalloc_tlsf.c
> > +++ b/xen/common/xmalloc_tlsf.c
> > @@ -140,9 +140,10 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl)
> >          *fl = flsl(*r) - 1;
> >          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
> >          *fl -= FLI_OFFSET;
> > -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> > -         *fl = *sl = 0;
> > -         */
> > +#if 0
> > +        if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
> > +        fl = *sl = 0;
> >       
> You want to get indentation right here, and you don't want to lose
> the indirection on fl.
>

Understood.

> > +#endif
> >          *r &= ~t;
> >      }
> >  }
> >       
> If you split this to 4 patches, leaving the URL proposal in just
> the cover letter, then I think this one change (with the adjustments)
> could go in right away. Similarly I expect the arm64/flushtlb.h
> change could be ack-ed right away by an Arm maintainer.
>     
    Ok. I do not understand what you mean by "leaving the URL proposal
    in just the cover letter". Which URL?

I'm sorry, I didn't notice your previous reply. I'm going to address your observations now:

Here "propose" is appropriate in the description, as this is something
the patch does not do. Further down, however, you mean to describe what
the patch does, not what an eventual patch might do.

To my knowledge, there is not a standard format to define a project deviation for a certain MISRA rule in Xen right now (this can also be discussed in a separate thread). To clarify, I meant to describe why I wasn't addressing these violations in the patch (they are the vast majority, but they do not have any implication w.r.t. functional safety and can therefore be safely deviated with an appropriate written justification).

> Somewhat similarly you don't want to use past tense in the title, as
> that wants to describe what is being done, rather than describing a
> patch that has already landed.

Understood.

> and it would avoid the somewhat odd splitting of adjacent comments
> (which then is at risk of someone later coming forward with a patch
> re-combining them).
>
> > --- a/xen/include/xen/atomic.h
> > +++ b/xen/include/xen/atomic.h
> > @@ -71,7 +71,10 @@ static inline void _atomic_set(atomic_t *v, int i);
> >   * Returns the initial value in @v, hence succeeds when the return value
> >   * matches that of @old.
> >   *
> > - * Sample (tries atomic increment of v until the operation succeeds):
> > + */
> > +/**
> > + *
> > + * Code sample: Tries atomic increment of v until the operation succeeds.
> >   *
> >   *  while(1)
> >   *  {
>
> Somewhat similarly here: I don't think the inner comment provides
> much value, and could hence be dropped instead of splitting the comment.
>

The rationale behind these modifications was to separate clearly comments and code samples, so that the latter can be easily deviated by tool configurations. I'm ok with the suggestion of removing the nested comments, and otherwise leave the code as is, but i'll probably do this by splitting the patch as you suggested above.

Regards,

  Nicola