Re: [PATCH 5/9] libxl: introduce libxl_cpupool_cpu{add, remove}_cpumap()

[Dario Faggioli <dario.faggioli@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2570 bytes --]

On Mon, 2015-03-09 at 10:39 +0000, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 06:21:32PM +0100, Dario Faggioli wrote:

> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -6343,17 +6343,31 @@ out:
> >  
> >  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu)
> >  {
> > +    GC_INIT(ctx);
> >      int rc;
> >  
> >      rc = xc_cpupool_addcpu(ctx->xch, poolid, cpu);
> >      if (rc) {
> > -        LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
> > -            "Error moving cpu to cpupool");
> > +        LOGE(ERROR, "Error moving cpu %d to cpupool", cpu);
> > +        GC_FREE;
> >          return ERROR_FAIL;
> >      }
> > +    GC_FREE;
> 
> Please use "goto" idiom. Same applies to libxl_cpupool_cpuremove.
> 
Ok.

> >      return 0;
> >  }
> >  
> > +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                const libxl_bitmap *cpumap)
> > +{
> > +    int c, ncpus = 0;
> > +
> > +    libxl_for_each_set_bit(c, *cpumap) {
> > +        if (!libxl_cpupool_cpuadd(ctx, poolid, c))
> > +            ncpus++;
> > +    }
> > +    return ncpus;
> > +}
> 
> I think returning a libxl error code on error and 0 on success is
> better. At least this stay in line with libxl_cpupool_cpuadd_node.
> 
Will do.

> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -1456,8 +1456,12 @@ int libxl_cpupool_destroy(libxl_ctx *ctx, uint32_t poolid);
> >  int libxl_cpupool_rename(libxl_ctx *ctx, const char *name, uint32_t poolid);
> >  int libxl_cpupool_cpuadd(libxl_ctx *ctx, uint32_t poolid, int cpu);
> >  int libxl_cpupool_cpuadd_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > +int libxl_cpupool_cpuadd_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                const libxl_bitmap *cpumap);
> >  int libxl_cpupool_cpuremove(libxl_ctx *ctx, uint32_t poolid, int cpu);
> >  int libxl_cpupool_cpuremove_node(libxl_ctx *ctx, uint32_t poolid, int node, int *cpus);
> > +int libxl_cpupool_cpuremove_cpumap(libxl_ctx *ctx, uint32_t poolid,
> > +                                   const libxl_bitmap *cpumap);
> >  int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid);
> >  int libxl_cpupool_info(libxl_ctx *ctx, libxl_cpupoolinfo *info, uint32_t poolid);
> >  
> 
> Missing #define LIBXl_HAVE_$FOO.
> 
So, do we need to do this every time we add a new function, even if not
changing any existing one, not adding fields to any data structure,
etc.?

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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