Re: [Xen-devel] [PATCH v6] libxl: provide libxl_bitmap_{or,and}

[Linda Jacobson <lindaj@xxxxxxxx>]

Sent from my iPhone

> On Apr 15, 2015, at 7:41 AM, Wei Liu <wei.liu2@xxxxxxxxxx> wrote:
> 
>> On Wed, Apr 15, 2015 at 02:29:10PM +0100, Ian Campbell wrote:
>>> 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".
> 
> Linda, can you use something like this in your commit log:
> 
> Provide libxl_bitmap_{and,or} functions. These functions can be used in
> vNUMA configuration check function. They are also generally useful so
> they are made as public API.


Of course

L
> 
>>>> 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?
> 
> OK, maybe I'm mistaken.
> 
> Linda, you need to add one more hunk to libxl.h.
> 
> #define LIBXL_HAVE_BITMAP_AND_OR 1
> 
> Search for other LIBXL_HAVE_ macro to get an idea how it is handled.
> 
> If you have any questions, just ask.
> 
> Wei.
> 
>> 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)
>>>>> {
>> 

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