Re: [PATCH v3 10/15] tools: implement the new libxl get hw info interface

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-09-19 11:28:18, Roger Pau Monn� wrote:
> On Tue, Sep 05, 2017 at 05:32:32PM +0800, Yi Sun wrote:
> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index dd412cc..d534ec2 100644
> > --- a/tools/libxl/libxl_psr.c
> > +++ b/tools/libxl/libxl_psr.c
> > @@ -382,60 +382,49 @@ static xc_psr_feat_type libxl__feat_type_to_libxc_feat_type(
> >      return xc_type;
> >  }
> >  
> > +static int libxl__hw_info_to_libxl_cat_info(
> > +               libxl_psr_feat_type type, libxl_psr_hw_info *hw_info,
> > +               libxl_psr_cat_info *cat_info)
> > +{
> > +    if (type != LIBXL_PSR_FEAT_TYPE_CAT)
> > +        return ERROR_INVAL;
> 
> Since this is an internal libxl function, is there any possible valid
> scenario where this function is called with type !=
> LIBXL_PSR_FEAT_TYPE_CAT?
> 
> If not this should be an assert instead, and the function could return
> void.
> 
Thanks for the good suggestion!

> > +
> > +    cat_info->id = hw_info->id;
> > +    cat_info->cos_max = hw_info->u.cat.cos_max;
> > +    cat_info->cbm_len = hw_info->u.cat.cbm_len;
> > +    cat_info->cdp_enabled = hw_info->u.cat.cdp_enabled;
> > +
> > +    return 0;
> > +}
> > +
> >  int libxl_psr_cat_get_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
> >                             unsigned int *nr, unsigned int lvl)
> >  {
[...]

> > -    libxl_for_each_set_bit(socketid, socketmap) {
> > -        ptr[i].id = socketid;
> > -        if (xc_psr_get_hw_info(ctx->xch, socketid, xc_type, &hw_info)) {
> > +    for (i = 0; i < *nr; i++) {
> > +        if (libxl__hw_info_to_libxl_cat_info(LIBXL_PSR_FEAT_TYPE_CAT,
> > +                                             &hw_info[i], &ptr[i])) {
> 
> Please use rc here:
> 
> rc = libxl__hw_info_to_libxl_cat_info(...);
> if (rc) {
>     ...
> 
> This has the bonus of not losing the error code returned by
> libxl__hw_info_to_libxl_cat_info.
> 
Ok.

> > +            libxl_psr_hw_info_list_free(hw_info, *nr);
> >              rc = ERROR_FAIL;
> >              free(ptr);
> >              goto out;
> >          }
> > -
> > -        ptr[i].cos_max = hw_info.u.xc_cat.cos_max;
> > -        ptr[i].cbm_len = hw_info.u.xc_cat.cbm_len;
> > -        ptr[i].cdp_enabled = hw_info.u.xc_cat.cdp_enabled;
> > -
> > -        i++;
> >      }
> >  
> >      *info = ptr;
> > -    *nr = i;
> > +    libxl_psr_hw_info_list_free(hw_info, *nr);
> >  out:
> > -    libxl_bitmap_dispose(&socketmap);
> >      GC_FREE;
> >      return rc;
> >  }
> > @@ -476,15 +465,97 @@ int libxl_psr_get_val(libxl_ctx *ctx, uint32_t domid,
> >      return ERROR_FAIL;
> >  }
> >  
[...]

> >  void libxl_psr_hw_info_list_free(libxl_psr_hw_info *list, unsigned int nr)
> >  {
> > +    unsigned int i;
> > +
> > +    for (i = 0; i < nr; i++)
> > +        libxl_psr_hw_info_dispose(&list[i]);
> > +    free(list);
> 
> Don't you also need a libxl_psr_cat_info_list_free? Or am I missing
> something?
> 
The libxl_psr_cat_info_list_free is called in xl_psr.c which already exists in
original codes.

> >  }
> >  
> >  /*
> > -- 
> > 1.9.1
> > 

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