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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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).

>  {
>      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.

> @@ -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.

> 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.

~Andrew