From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linda Subject: Re: [PATCH] new functions libxl_bitmap_{or,and} Date: Tue, 14 Apr 2015 06:14:45 -0600 Message-ID: <552D04B5.7050302@jma3.com> References: <1428911238-6244-1-git-send-email-lindaj@jma3.com> <20150414091942.GN17670@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150414091942.GN17670@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: ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, lars.kurth.xen@gmail.com, xen-devel@lists.xen.org, julien.grall@citrix.com, ian.jackson@citrix.com List-Id: xen-devel@lists.xenproject.org On 4/14/2015 3:19 AM, Wei Liu wrote: > Thanks, we're getting there. If my comments confuse you please just ask > for clarification. > > There is no need to change the subject line. However, it would be > useful if you have some kind of version number in you subject line. That > is > > [PATCH vX] libxl: provide libxl_bimap_{and,or} > > You can do this by supplying --subject-prefix= to git format-patch > > git format-patch -1 --subject-prefix="[PATCH vX]" ... What version do you want me to use at this point? I've sort of lost count, since many changes have been style changes. I am assuming you usually reserve new versions for substantive changes? > > where X refers to your version number. > > On Mon, Apr 13, 2015 at 01:47:18AM -0600, Linda Jacobson wrote: >> provide logical and and or of two bitmaps >> > And the SoB line should be here. What does SoB stand for in this context? > >> --- >> >> v.1 updated comments and format >> v.2 rewrote bitmap functions to manipulate bytes not bits >> >> Signed-off-by: Linda Jacobson >> --- >> tools/libxl/libxl_utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_utils.h | 5 ++++ >> 2 files changed, 79 insertions(+) >> >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >> index 9053b27..5c7178f 100644 >> --- a/tools/libxl/libxl_utils.c >> +++ b/tools/libxl/libxl_utils.c >> @@ -691,6 +691,80 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit) >> bitmap->map[bit / 8] &= ~(1 << (bit & 7)); >> } >> >> +/* provide logical or and logical and of two bitmaps */ > Normally we take the line before as the comment for the function that > follows it. So you don't need to mention "and" function here. > > Actually I don't think you need a comment for such obvious function at > all. I'll take it out. > >> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map, >> + libxl_bitmap *map1, libxl_bitmap *map2) > Actually I think you need to constify map1 and map2. I.e. > > const libxl_bitmap *map1, > const libxl_bitmap *map2) How come? Out of curiosity. > > > Sorry for not having mentioned these earlier. Since you're going to > resend due to other issues so you might as well just take care of these > nits. > >> +{ >> + GC_INIT(ctx); >> + int rc; >> + uint32_t i; >> + libxl_bitmap *lgmap; >> + libxl_bitmap *smap; >> + > Constify these as well. > > And probably use better name like large_map and small_map? OK. Happy to. Earlier someone complained about my names being too verbose. Linda > >> + if (map1->size > map2->size) { >> + lgmap = map1; >> + smap = map2; >> + } >> + else { > Coding style. Should be > > } else { > >> + lgmap = map2; >> + smap = map1; >> + } >> + >> + rc = libxl_bitmap_alloc(ctx, or_map, lgmap->size * 8); >> + if (rc) >> + goto out; >> + >> + /* >> + * if bitmaps aren't the same size, their union (logical or) will > If > >> + * be size of larger bit map. Any bit past the end of the >> + * smaller bit map, will match the larger one. >> + */ >> + for (i = 0; i < smap->size; i++) >> + or_map->map[i] = (smap->map[i] | lgmap->map[i]); >> + >> + for (i = smap->size; i < lgmap->size; i++) >> + or_map->map[i] = lgmap->map[i]; >> + >> +out: >> + GC_FREE; >> + return rc; >> +} >> + >> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map, >> + libxl_bitmap *map1, libxl_bitmap *map2) > Constify map1 and map2. > >> +{ >> + GC_INIT(ctx); >> + int rc; >> + uint32_t i; >> + libxl_bitmap *lgmap, *smap; > Constify lgmap and smap. > >> + >> + if (map1->size > map2->size) { >> + lgmap = map1; >> + smap = map2; >> + } >> + else { > Coding style. > >> + lgmap = map2; >> + smap = map1; >> + } >> + >> + >> + rc = libxl_bitmap_alloc(ctx, and_map, smap->size * 8); >> + if (rc) >> + goto out; >> + >> + /* >> + * if bitmaps aren't same size, their 'and' will be size of > If > >> + * smaller bit map >> + */ >> + for (i = 0; i < and_map->size; i++) >> + and_map->map[i] = (lgmap->map[i] & smap->map[i]); >> + >> +out: >> + GC_FREE; >> + return rc; >> + >> +} >> + >> int libxl_bitmap_count_set(const libxl_bitmap *bitmap) >> { >> int i, nr_set_bits = 0; >> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h >> index acacdd9..1c0086b 100644 >> --- a/tools/libxl/libxl_utils.h >> +++ b/tools/libxl/libxl_utils.h >> @@ -90,6 +90,11 @@ 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 */ > Or > >> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map, >> + libxl_bitmap *map1, libxl_bitmap *map2); >> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map, >> + libxl_bitmap *map1, libxl_bitmap *map2); > Constify map1 and map2. > > > Wei. > >> char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap); >> static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap) >> { >> -- >> 1.9.1