From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linda Subject: Re: [PATCH] tools/libxl new bitmap functions Date: Tue, 07 Apr 2015 09:45:11 -0600 Message-ID: <5523FB87.4070803@jma3.com> References: <1427996296-27980-1-git-send-email-lindaj@jma3.com> <20150402193432.GJ3131@x230.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150402193432.GJ3131@x230.dumpdata.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: Konrad Rzeszutek Wilk Cc: julien.grall@citrix.com, xen-devel@lists.xenproject.org, lars.kurth@xen.org, wei.liu2@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org Hey Konrad, On 4/2/2015 1:34 PM, Konrad Rzeszutek Wilk wrote: > On Thu, Apr 02, 2015 at 11:38:16AM -0600, Linda Jacobson wrote: >> From: Linda >> >> Added bitmap functions for union intersection and difference betweenn two bitmaps >> >> Signed-off-by: Linda >> --- >> tools/libxl/libxl_utils.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++ >> tools/libxl/libxl_utils.h | 10 ++++ >> 2 files changed, 125 insertions(+) >> >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >> index 9053b27..c390ddc 100644 >> --- a/tools/libxl/libxl_utils.c >> +++ b/tools/libxl/libxl_utils.c >> @@ -699,6 +699,121 @@ int libxl_bitmap_count_set(const libxl_bitmap *bitmap) >> >> return nr_set_bits; >> } >> +int libxl_bitmap_union(libxl_ctx *ctx, libxl_bitmap *union_bitmap, > You have an extra space at the end.. >> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2) > And this should really start at the same line length as 'libxl_ctx' > > Ditto for the rest of the functions. >> +{ >> + int size; >> + int rc; >> + >> + GC_INIT(ctx); >> + >> +// if bitmaps aren't the same size, union should be size of larger bit map > The comments are in: > > /* Comment here. */ > > >> + size = (bitmap1->size > bitmap2->size) ? bitmap1->size : bitmap2->size; > There might be an 'max' macro somwhere in the code that you could use > for this? Any guesses on where I might find the max macro. When I grep max, all I find are definitions of constants that are the max of something. Thanks. Linda > >> + >> + libxl_bitmap_init(union_bitmap); >> + rc = libxl_bitmap_alloc(ctx, union_bitmap, size); >> + if (rc) >> + { >> + // Following the coding standards here. >> + //First goto I've written in decades. > Hehe. > > No need to add the braces, you can just do: > > if (rc) > goto out; > >> + goto out; >> + } >> + >> + for (int bit = 0; bit < (size * 8); bit++) > Please move the 'int bit' to the start of the function. > >> + { >> + // libxl_bitmap_test returns 0 if past end of bitmap >> + // if the bit is set in either bitmap, set it in their union > I would change that comment to be: > > /* NB. libxl_bitmap_test returns 0 if past the end of bitmap. */ > >> + if (libxl_bitmap_test(bitmap1, bit)) >> + { >> + libxl_bitmap_set(union_bitmap, bit); >> + } >> + else if (libxl_bitmap_test(bitmap2, bit)) >> + { >> + libxl_bitmap_set(union_bitmap, bit); >> + } > You can ditch the braces. > >> + } >> + >> +out: >> + GC_FREE; >> + return rc; >> +} >> + >> +int libxl_bitmap_intersection (libxl_ctx *ctx, libxl_bitmap > There is space : ^ - which should not be there. > >> +*intersection_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2) >> +{ >> + int size; >> + int rc; >> + >> + GC_INIT(ctx); >> + >> +// if bitmaps aren't same size, intersection should be size of >> +// smaller bit map > I believe the comments I've above apply to the code below as well. > >> + size = (bitmap1->size > bitmap2->size) ? bitmap2->size : bitmap1->size; >> + >> + libxl_bitmap_init(intersection_bitmap); >> + rc = libxl_bitmap_alloc(ctx, intersection_bitmap, size); >> + if (rc) >> + { >> + goto out; >> + } >> + >> + for (int bit = 0; bit < (size * 8); bit++) >> + { >> + // libxl_bitmap_test returns 0 if past end of bitmap >> + // if the bit is set in both bitmaps, set it in their intersection >> + if (libxl_bitmap_test (bitmap1, bit) && >> + libxl_bitmap_test (bitmap2, bit) ) > You have an extra space at the end there. Don't think you need that > in libxl code (thought you do for libxc). Yeah, two different > styleguides! > >> + { >> + libxl_bitmap_set (intersection_bitmap, bit); >> + } >> + } >> + >> +out: >> + GC_FREE; >> + return rc; >> +} >> +int libxl_bitmap_difference(libxl_ctx *ctx, libxl_bitmap *difference_bitmap, >> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2) >> +{ >> + int size; >> + int rc; >> + >> + GC_INIT(ctx); >> + >> +// if bitmaps aren't the same size, difference should be size of larger >> + size = (bitmap1->size > bitmap2->size) ? bitmap1->size : bitmap2->size; >> + >> + libxl_bitmap_init(difference_bitmap); >> + rc = libxl_bitmap_alloc(ctx, difference_bitmap, size); >> + if (rc) >> + { >> + goto out; >> + } >> + >> + for (int bit = 0; bit < (size * 8); bit++) >> + { >> + /* libxl_bitmap_test returns 0 if past end of bitmap >> + if the bit is set in one bitmap and not the other, set it in >> +their difference >> + NOTE: if one bit map is larger, this will result in all bits >> +being set past the size of the smaller bitmap; if this is not >> + the desired behavior, please let me know > That would make this a bit strange. Perhaps demand that they > must be the same size? If they are not then just return an error? >> + */ >> + >> + if (libxl_bitmap_test (bitmap1, bit) > You have an extra space at the end there. ^ >> + && (!libxl_bitmap_test (bitmap2, bit)) ) >> + { >> + libxl_bitmap_set (difference_bitmap, bit); >> + } >> + } >> + >> +out: >> + GC_FREE; >> + return rc; >> +} >> + >> + >> + >> >> /* NB. caller is responsible for freeing the memory */ >> char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *bitmap) >> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h >> index acacdd9..521e4bb 100644 >> --- a/tools/libxl/libxl_utils.h >> +++ b/tools/libxl/libxl_utils.h >> @@ -91,6 +91,16 @@ 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); >> char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap); >> +/* >> + union, intersection and difference functions for >> + two bitmaps >> +*/ >> +int libxl_bitmap_union(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2); >> + >> +int libxl_bitmap_intersection(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2); >> + >> +int libxl_bitmap_difference(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2); >> + >> static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap) >> { >> memset(bitmap->map, -1, bitmap->size); >> -- >> 1.9.1 >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel