From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linda Subject: Re: [PATCH] libxl: provide libxl_bitmap_{and,or} Date: Sun, 12 Apr 2015 13:24:47 -0600 Message-ID: <552AC67F.6070308@jma3.com> References: <1428636847-30584-1-git-send-email-lindaj@jma3.com> <5529848E.60607@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5529848E.60607@citrix.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: Julien Grall , xen-devel@lists.xen.org Cc: Lars Kurth , Ian Jackson , Wei Liu , Ian Campbell , Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Hi all, Just for grins, I created and unit tested a little code snippet for 'and' and 'or', using bytes instead of bits. It didn't take that long. If you want me to replace the code I submitted with this new code, I'd be happy to. Otherwise, I'll just make the style changes Julian requested, (unless there is a disagreement on where the GC_INIT should be), and merge the changes. Linda On 4/11/2015 2:31 PM, Julien Grall wrote: > Hi Linda, > > Thank you for sending the new version. > > It's common to CC the maintainers of the patch. Most of us have filter > in order to get directly mail they are involved too and avoid watching > every time the ML. Otherwise answer to your mail may take longer. > > You can find them with the scripts scripts/get_maintainers.pl. I've > cced them. > > On 10/04/2015 04:34, Linda Jacobson wrote: >> Added functions to create the logical 'and' and logical 'or' >> of two input bitmaps. Cleaned up spacing and comments. >> >> Removed accidentally committed libxl_u_disk* files. > > Thank you for saying what you changed in this version. While it's > useful during review process, this is not necessary to keep them in > the final commit message. > > When the committers will commit your patch, git will ignore everything > after "---" and a newline. > > For instance you could write > > --- > Changes in v3: > - Foo ... > > Changes in v2: > - Added functions... > - Removed ... > > >> Signed-off-by: Linda Jacobson >> --- >> tools/libxl/libxl_utils.c | 55 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_utils.h | 6 ++++++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >> index 9053b27..fb8830a 100644 >> --- a/tools/libxl/libxl_utils.c >> +++ b/tools/libxl/libxl_utils.c >> @@ -691,6 +691,61 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, >> int bit) >> bitmap->map[bit / 8] &= ~(1 << (bit & 7)); >> } >> >> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map, >> + libxl_bitmap *map1, libxl_bitmap *map2) >> +{ >> + uint32_t size; >> + int rc; >> + int bit; >> + >> + GC_INIT(ctx); > > GC_INIT is a macro which declaring a variable. It seems common to > declare at the very beginning of the function. Can you move it there? > >> + /* if bitmaps aren't same size, the bitmap of their logical >> + or should be the size of the larger bit map >> + */ > > The coding style for multi-lines comment is: > > /* > * My comment > * continue > */ > >> + size = max(map1->size, map2->size); >> + rc = libxl_bitmap_alloc(ctx, or_map, size); >> + if (rc) >> + goto out; >> + >> + for (bit = 0; bit < (size * 8); bit++) { >> + if (libxl_bitmap_test(map1, bit)) >> + libxl_bitmap_set(or_map, bit); >> + else if (libxl_bitmap_test(map2, bit)) >> + libxl_bitmap_set(or_map, bit); >> + } >> + >> +out: >> + GC_FREE; >> + return rc; >> +} >> + >> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map, >> + libxl_bitmap *map1, libxl_bitmap *map2) >> +{ >> + uint32_t size; >> + int rc; >> + int bit; >> + >> + GC_INIT(ctx); > > Same remark. > >> + /* if bitmaps aren't same size, logical and should >> + be the size of the smaller bit map >> + */ > > Coding style > >> + size = min(map1->size, map2->size); >> + rc = libxl_bitmap_alloc(ctx, and_map, size); >> + if (rc) >> + goto out; >> + >> + for (bit = 0; bit < (size * 8); bit++) { >> + if (libxl_bitmap_test (map1, bit) && >> + libxl_bitmap_test (map2, bit) ) >> + libxl_bitmap_set (and_map, bit); >> + } >> + >> +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 68b5580..0b6480d 100644 >> --- a/tools/libxl/libxl_utils.h >> +++ b/tools/libxl/libxl_utils.h >> @@ -91,6 +91,12 @@ 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 *bitmap); >> char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap >> *bitmap); >> +/* or, and and xor functions for two bitmaps >> + */ > > I saw you sent a separate patch for fixing the coding style. I don't > think it's necessary. Can you merge the 2 patches? > > Although, the coding style for a one line comment is > /* My comment */ > >> +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); >> static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap) >> { >> memset(bitmap->map, -1, bitmap->size); >> > > Other than few coding style issue, I think the 2 functions (or & and) > can be optimize (i.e working with byte rather than bit). Although, I'm > not a maintainers so I will let them decide if it's worth to do it. > > Regards, >