Re: [XEN PATCH v2 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 31/07/2023 13:21, Jan Beulich wrote:
> On 31.07.2023 13:15, Jan Beulich wrote:
> > On 31.07.2023 09:33, Nicola Vetrini wrote:
> > > --- a/xen/arch/x86/include/asm/hvm/save.h
> > > +++ b/xen/arch/x86/include/asm/hvm/save.h
> > > @@ -49,21 +49,21 @@ void _hvm_read_entry(struct hvm_domain_context 
> > > *h,
> > >   */
> > >  #define _hvm_load_entry(_x, _h, _dst, _strict) ({                    
> > >    \
> > >      int r;                                                           
> > >    \
> > > -    struct hvm_save_descriptor *desc                                 
> > >    \
> > > +    struct hvm_save_descriptor *descriptor                           
> > >    \
> > >          = (struct hvm_save_descriptor *)&(_h)->data[(_h)->cur];      
> > >    \
> > >      if ( (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),              
> > >    \
> > >                 HVM_SAVE_LENGTH(_x), (_strict))) == 0 )               
> > >    \
> > >      {                                                                
> > >    \
> > >          _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH(_x));          
> > >    \
> > >          if ( HVM_SAVE_HAS_COMPAT(_x) &&                              
> > >    \
> > > -             desc->length != HVM_SAVE_LENGTH(_x) )                   
> > >    \
> > > -            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);       
> > >    \
> > > +             descriptor->length != HVM_SAVE_LENGTH(_x) )             
> > >    \
> > > +            r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length); 
> > >    \
> > >      }                                                                
> > >    \
> > >      else if (HVM_SAVE_HAS_COMPAT(_x)                                 
> > >    \
> > >               && (r = _hvm_check_entry((_h), HVM_SAVE_CODE(_x),       
> > >    \
> > >                         HVM_SAVE_LENGTH_COMPAT(_x), (_strict))) == 0 
> > > ) { \
> > >          _hvm_read_entry((_h), (_dst), HVM_SAVE_LENGTH_COMPAT(_x));   
> > >    \
> > > -        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), desc->length);           
> > >    \
> > > +        r = HVM_SAVE_FIX_COMPAT(_x, (_dst), descriptor->length);     
> > >    \
> > >      }                                                                
> > >    \
> > >      r; })
> > The macro-local variable gets too long for my taste, to be honest,
> > and it being improperly named anyway suggests it simply wants a
> > trailing underscore added. And then, since for a variable named "r"
> > the risk of shadowing is equally high, that one wants to gain a
> > trailing underscore as well imo. (And while at it, I personally
> > would also drop the leading underscores from the macro parameter
> > names. Furthermore I think it would be nice if at on the lines
> > touched anyway indentation was also corrected. Overall maybe best
> > if I submit a patch.)
> In that replacement patch I would like to mention what "desc" this
> collides with, but your description didn't say so and I'm afraid I
> also haven't been able to spot it (grep-ing for "desc", even with
> a couple of extra restrictions, still yields way too many hits).
>
> Jan
It's the local variable in 'xen/arch/x86/hvm/save.c:281', but since the 
macro
is used elsewhere quite a few times, doing a suitable rename inside it
(that does not shadow anything) prevents other users from possibly 
shadowing it
in the future. Your points are all agreeable in my opinion. Since your 
concerns
with this macro are also about issues other than MISRA violations, I 
agree that
this patch should be dropped.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)