Re: [Xen-devel] [PATCH v4 3/4] tools: add tools support for Intel CDP

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-09-17 at 11:38 +0100, Andrew Cooper wrote:
> On 17/09/15 10:35, He Chen wrote:
> > @@ -2798,7 +2798,9 @@ enum xc_psr_cmt_type {
> >  typedef enum xc_psr_cmt_type xc_psr_cmt_type;
> >  
> >  enum xc_psr_cat_type {
> > -    XC_PSR_CAT_L3_CBM = 1,
> > +    XC_PSR_CAT_L3_CBM  = 1,
> > +    XC_PSR_CAT_L3_CODE = 2,
> > +    XC_PSR_CAT_L3_DATA = 3,
> >  };
> 
> No need for the explicit assignments here.  The exact values are not
> interesting and guaranteed to be as currently assigned.

No harm though I suppose.

> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index ebbb9a5..8128f54 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -8390,6 +8390,10 @@ static int psr_cat_hwinfo(void)
> >          }
> >          printf("%-16s: %u\n", "Socket ID", socketid);
> >          printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
> > +        if (info->cdp_enabled)
> > +            printf("%-16s: Enabled\n", "CDP Status");
> > +        else
> > +            printf("%-16s: Disabled\n", "CDP Status");
> 
> printf("%-16s: %sabled\n", "CDP Status", info->cdp_enabled ? "En" :
> "Dis");
> 
> is rather shorter, if you prefer.

My preference would be along these lines but without the trick regarding
the common suffix on both words, i.e.

printf("%-16s: %s\n", "CDP Status",
       info->cdp_enabled ? "Enabled" : "Disabled");

As well as avoiding splitting the words this avoids duplicating the %-16s
and "CDP Status" compared with the original, which IMHO is the important
thing.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel