Re: [PATCH v4 04/11] xsm: apply coding style

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 9/6/21 2:17 PM, Andrew Cooper wrote:
> On 03/09/2021 20:06, Daniel P. Smith wrote:
> > Instead of intermixing coding style changes with code changes as they
> > are come upon in this patch set, moving all coding style changes
> > into a single commit. The focus of coding style changes here are,
> >
> >   - move trailing comments to line above
> >   - ensuring line length does not exceed 80 chars
> >   - ensuring proper indentation for 80 char wrapping
> >   - covert u32 type statements to  uint32_t
> >   - remove space between closing and opening parens
> >   - drop extern on function declarations
> >
> > Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> > ---
> >   xen/include/xsm/dummy.h | 173 +++++++++-----
> >   xen/include/xsm/xsm.h   | 494 ++++++++++++++++++++++------------------
> >   xen/xsm/xsm_core.c      |   4 +-
> >   3 files changed, 389 insertions(+), 282 deletions(-)
> >
> > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> > index 214b5408b1..deaf23035e 100644
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -69,8 +69,9 @@ void __xsm_action_mismatch_detected(void);
> >   
> >   #endif /* CONFIG_XSM */
> >   
> > -static always_inline int xsm_default_action(
> > -    xsm_default_t action, struct domain *src, struct domain *target)
> > +static always_inline int xsm_default_action(xsm_default_t action,
> > +                                            struct domain *src,
> > +                                            struct domain *target)
> The old code is correct.  We have plenty of examples of this in Xen, and
> I have been adding new ones when appropriate.
>
> It avoids squashing everything on the RHS and ballooning the line count
> to compensate.  (This isn't a particularly bad example, but we've had
> worse cases in the past).
Based on the past discussions I understood either is acceptable and find 
this version much easier to visually parse myself. With that said, if
the "next line single indent" really is the preferred style by the 
maintainers/community, then I can convert all of these over.
> >   {
> >       switch ( action ) {
> >       case XSM_HOOK:
> > @@ -99,12 +100,13 @@ static always_inline int xsm_default_action(
> >   }
> >   
> >   static XSM_INLINE void xsm_security_domaininfo(struct domain *d,
> > -                                    struct xen_domctl_getdomaininfo *info)
> > +    struct xen_domctl_getdomaininfo *info)
> This doesn't match any styles I'm aware of.  Either struct domain on the
> new line, or the two structs vertically aligned.
>
> It more obviously highlights why squashing all parameters on the RHS is
> a bad move.
Apologies I let one slip through, though going through over 80-some 
function defs trying to make sure they are all aligned and missing one
I would say is not a bad job.

> > @@ -291,37 +307,41 @@ static XSM_INLINE void xsm_evtchn_close_post(struct 
> > evtchn *chn)
> >       return;
> >   }
> >   
> > -static XSM_INLINE int xsm_evtchn_send(XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn)
> > +static XSM_INLINE int xsm_evtchn_send(XSM_DEFAULT_ARG struct domain *d,
> > +                                      struct evtchn *chn)
> >   {
> >       XSM_ASSERT_ACTION(XSM_HOOK);
> >       return xsm_default_action(action, d, NULL);
> >   }
> >   
> > -static XSM_INLINE int xsm_evtchn_status(XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn)
> > +static XSM_INLINE int xsm_evtchn_status(XSM_DEFAULT_ARG struct domain *d,
> > +                                        struct evtchn *chn)
> >   {
> >       XSM_ASSERT_ACTION(XSM_TARGET);
> >       return xsm_default_action(action, current->domain, d);
> >   }
> >   
> > -static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1, struct domain *d2)
> > +static XSM_INLINE int xsm_evtchn_reset(XSM_DEFAULT_ARG struct domain *d1,
> > +                                       struct domain *d2)
> >   {
> >       XSM_ASSERT_ACTION(XSM_TARGET);
> >       return xsm_default_action(action, d1, d2);
> >   }
> >   
> > -static XSM_INLINE int xsm_alloc_security_evtchns(
> > -    struct evtchn chn[], unsigned int nr)
> > +static XSM_INLINE int xsm_alloc_security_evtchns(struct evtchn chn[],
> > +                                                 unsigned int nr)
> I maintain that this was correct before.
Getting to this point I must say it would be helpful if this could be 
spelled out in CODING_STYLE. Specifically, so that I am clear, if a 
parameter overflows, than all the parameters overflow? Are there 
exceptions such as if overflow doesn't happen until the third of four or 
the fourth parameter. Having a rule set would be much more helpful than 
trying to look for examples elsewhere in code.
> > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> > index 9872bae502..8878281eae 100644
> > --- a/xen/include/xsm/xsm.h
> > +++ b/xen/include/xsm/xsm.h
> > @@ -19,7 +19,7 @@
> >   #include <xen/multiboot.h>
> >   
> >   /* policy magic number (defined by XSM_MAGIC) */
> > -typedef u32 xsm_magic_t;
> > +typedef uint32_t xsm_magic_t;
> >   
> >   #ifdef CONFIG_XSM_FLASK
> >   #define XSM_MAGIC 0xf97cff8c
> > @@ -31,158 +31,171 @@ typedef u32 xsm_magic_t;
> >    * default actions of XSM hooks. They should be compiled out otherwise.
> >    */
> >   enum xsm_default {
> > -    XSM_HOOK,     /* Guests can normally access the hypercall */
> > -    XSM_DM_PRIV,  /* Device model can perform on its target domain */
> > -    XSM_TARGET,   /* Can perform on self or your target domain */
> > -    XSM_PRIV,     /* Privileged - normally restricted to dom0 */
> > -    XSM_XS_PRIV,  /* Xenstore domain - can do some privileged operations */
> > -    XSM_OTHER     /* Something more complex */
> > +    /* Guests can normally access the hypercall */
> > +    XSM_HOOK,
> > +    /* Device model can perform on its target domain */
> > +    XSM_DM_PRIV,
> > +    /* Can perform on self or your target domain */
> > +    XSM_TARGET,
> > +    /* Privileged - normally restricted to dom0 */
> > +    XSM_PRIV,
> > +    /* Xenstore domain - can do some privileged operations */
> > +    XSM_XS_PRIV,
> > +    /* Something more complex */
> > +    XSM_OTHER
> >   };
> Why?  This takes a table which was unambiguous to read, and makes it
> ambiguous at a glance.  You want either no change at all, or blank lines
> between comment/constant pairs so you don't need to read to either end
> to figure out how to parse the comments.
I went back to the comment that prompted me to do this and rereading now 
makes me think I took it to literal. Specifically, "...I'd like to 
encourage you to also address other style issues in the newly introduced 
file. Here I'm talking about comment style, requiring /* to be on its 
own line." I took that to include these as well since I am pretty sure I 
have seen elsewhere this kind of commenting. Regardless, looking back I 
 can see how the meaning could have only for other block quotes. 
Honestly I will gladly change it back as I think that is far clearer.
v/r,
dps