From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v6] libxl: provide libxl_bitmap_{or,and} Date: Wed, 15 Apr 2015 14:29:10 +0100 Message-ID: <1429104550.15516.287.camel@citrix.com> References: <1429098311-30180-1-git-send-email-lindaj@jma3.com> <1429101914.15516.262.camel@citrix.com> <20150415131550.GC8128@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YiNNX-0007qG-Iu for xen-devel@lists.xenproject.org; Wed, 15 Apr 2015 13:29:19 +0000 In-Reply-To: <20150415131550.GC8128@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: julien.grall@citrix.com, xen-devel@lists.xenproject.org, ian.jackson@citrix.com, stefano.stabellini@eu.citrix.com, Linda Jacobson List-Id: xen-devel@lists.xenproject.org On Wed, 2015-04-15 at 14:15 +0100, Wei Liu wrote: > On Wed, Apr 15, 2015 at 01:45:14PM +0100, Ian Campbell wrote: > > On Wed, 2015-04-15 at 05:45 -0600, Linda Jacobson wrote: > > > There are new functions to provide logical and and or of two bitmaps. > > > > Please could you add a sentence or two on the intended use of these > > functions, since there are no callers being added here. > > > > Linda is our Outreachy applicant. This is a small task that Julien and I > assigned to her. Sure, but that doesn't remove the need for the commit log to be a standalone justification for the patch in its own right. > One user I can think of is in some of the vNUMA validation functions > that operate on bitmaps. But to keep this task small and simple I didn't > ask her to actually use the functions she introduce. > > > In particular without that I can't tell if these need to be part of the > > public API or if they are going to be used by something internal. > > > > I think these functions should be public functions. Sure, but the reasoning for why you^WLinda thinks that needs to be in the commit log. In particular because there are no users being added. I could probably guess why you think these should be public, but I shouldn't have to guess and in any case that doesn't help in 6 months when someone asks "why do we have these functions". > > If the former then I think a LIBXL_HAVE_... #define is needed in libxl.h > > (as described by the comments in there, and there are many examples). > > > > We're a bit lax on these functions (there is no LIBXL_HAVE_BITMAP...) Weren't they always there and hence don't need it? Ian. > so > I didn't ask her to add one. We can add LIBXL_HAVE_BITMAP_AND_OR in > libxl.h if you think it is necessary. > > Wei. > > > If the latter then the prototype should differ. (I'll explain that > > if/when this turns out to be the case). > > > > > diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h > > > index acacdd9..a128a7c 100644 > > > --- a/tools/libxl/libxl_utils.h > > > +++ b/tools/libxl/libxl_utils.h > > > @@ -90,6 +90,13 @@ int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit); > > > void libxl_bitmap_set(libxl_bitmap *bitmap, int bit); > > > void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit); > > > int libxl_bitmap_count_set(const libxl_bitmap *cpumap); > > > +/* Or and and functions for two bitmaps */ > > > > This comment doesn't say anything which isn't already apparent from the > > names and prototypes of the functions in question. Just leave it out. > > > > > > > +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map, > > > + const libxl_bitmap *map1, > > > + const libxl_bitmap *map2); > > > +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map, > > > + const libxl_bitmap *map1, > > > + const libxl_bitmap *map2); > > > char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap); > > > static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap) > > > { > >