Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

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

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

On Mon, 2015-09-28 at 15:12 +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:

> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5f9047c..5a91687 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> >  
> >  /*
> > - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> > + * LIBXL_HAVE_SOCKET_BITMAP
> >   *
> > - * If this is defined, then libxl_socket_bitmap_alloc exists.
> > + * If this is defined, then libxl_socket_bitmap_alloc and
> > + * libxl_socket_bitmap_Fill exist.
> 
> _Fill -> _fill.
> 
Right.

However, it seems to me that the function would be better named
libxl_get_online_sockets() or something like that.

I see that you want the actual map. For CPUs and NUMA nodes, we do have
libxl_get_online_{cpus,nodes}(), but they return the number of online
CPUs and nodes, so for consistency, libxl_get_online_sockets() would
better (if necessary at some point) behave similarly.

What about libxl_get_online_socketmap() ?  This is still a nice (IMO)
name for what you're after here, and it leaves us room to implement
libxl_get_online_cpumap() and libxl_get_online_nodemap(), if we'll ever
need those.

Thoughs?

> >   */
> > -#define LIBXL_HAVE_SOCKET_BITMAP_ALLOC 1
> > +#define LIBXL_HAVE_SOCKET_BITMAP 1
> >  
> 
> Normally we don't / can't delete existing macros. However this macro
> was
> introduced in this cycle, so we have the liberty to change it before
> releasing.
> 
I was about to say exactly the same thing.

> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index bfc9699..557f279 100644
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c

> > +int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap
> > *socketmap)
> > +{
> > +    libxl_cputopology *tinfo = NULL;
> > +    int nr_cpus = 0, i, rc = 0;
> > +
> > +    tinfo = libxl_get_cpu_topology(ctx, &nr_cpus);
> > +    if (tinfo == NULL) {
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    libxl_bitmap_set_none(socketmap);
> > +    for (i = 0; i < nr_cpus; i++)
> > +        if (tinfo[i].socket != XEN_INVALID_SOCKET_ID
> > +            && !libxl_bitmap_test(socketmap, tinfo[i].socket))
> > +            libxl_bitmap_set(socketmap, tinfo[i].socket);
> > + out:
> > +    libxl_cputopology_list_free(tinfo, nr_cpus);
> > +    return rc;
> > +
> > +}
> 
> Need extra blank line.
> 
And, at the same time, the blank line between 'return rc;' and the '{'
is not needed. IOW, we want this:

   ...
   libxl_cputopology_list_free(tinfo, nr_cpus);
   return rc;
 }

 int libxl_nodemap_to_cpumap(...
 ...

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


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