| Message ID | 20190620010356.19164-5-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 20.06.19 03:03, John Snow wrote: > Nobody calls the function like this currently, but we neither prohibit > or cope with this behavior. I decided to make the function cope with it. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > util/hbitmap.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/util/hbitmap.c b/util/hbitmap.c > index 7905212a8b..45d1725daf 100644 > --- a/util/hbitmap.c > +++ b/util/hbitmap.c > @@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b) > } > > /** > - * Given HBitmaps A and B, let A := A (BITOR) B. > - * Bitmap B will not be modified. > + * Given HBitmaps A and B, let R := A (BITOR) B. > + * Bitmaps A and B will not be modified, > + * except when bitmap R is an alias of A or B. > * > * @return true if the merge was successful, > * false if it was not attempted. > @@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) > } > assert(hbitmap_can_merge(b, result)); > > - if (hbitmap_count(b) == 0) { > + if ((!hbitmap_count(a) && result == b) || > + (!hbitmap_count(b) && result == a) || > + (!hbitmap_count(a) && !hbitmap_count(b))) { > return true; > } The rest of this function completely overwrites the @result bitmap. Therefor, @result does not need to be cleared when calling this function. Therfore, hbitmap_merge(hbitmap_alloc(), hbitmap_alloc(), output) should actually clear @output, I think. Max
On 6/20/19 11:39 AM, Max Reitz wrote: > On 20.06.19 03:03, John Snow wrote: >> Nobody calls the function like this currently, but we neither prohibit >> or cope with this behavior. I decided to make the function cope with it. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> util/hbitmap.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index 7905212a8b..45d1725daf 100644 >> --- a/util/hbitmap.c >> +++ b/util/hbitmap.c >> @@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b) >> } >> >> /** >> - * Given HBitmaps A and B, let A := A (BITOR) B. >> - * Bitmap B will not be modified. >> + * Given HBitmaps A and B, let R := A (BITOR) B. >> + * Bitmaps A and B will not be modified, >> + * except when bitmap R is an alias of A or B. >> * >> * @return true if the merge was successful, >> * false if it was not attempted. >> @@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) >> } >> assert(hbitmap_can_merge(b, result)); >> >> - if (hbitmap_count(b) == 0) { >> + if ((!hbitmap_count(a) && result == b) || >> + (!hbitmap_count(b) && result == a) || >> + (!hbitmap_count(a) && !hbitmap_count(b))) { >> return true; >> } > > The rest of this function completely overwrites the @result bitmap. > Therefor, @result does not need to be cleared when calling this function. > > Therfore, hbitmap_merge(hbitmap_alloc(), hbitmap_alloc(), output) should > actually clear @output, I think. > > Max > Ah, wellp, you're right. That'd be the second problem with this function. It used to be strictly A = A | B, but we changed it -- very incompletely -- to R = A | B; which explains these two bugs. Thanks.
diff --git a/util/hbitmap.c b/util/hbitmap.c index 7905212a8b..45d1725daf 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -781,8 +781,9 @@ bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b) } /** - * Given HBitmaps A and B, let A := A (BITOR) B. - * Bitmap B will not be modified. + * Given HBitmaps A and B, let R := A (BITOR) B. + * Bitmaps A and B will not be modified, + * except when bitmap R is an alias of A or B. * * @return true if the merge was successful, * false if it was not attempted. @@ -797,7 +798,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) } assert(hbitmap_can_merge(b, result)); - if (hbitmap_count(b) == 0) { + if ((!hbitmap_count(a) && result == b) || + (!hbitmap_count(b) && result == a) || + (!hbitmap_count(a) && !hbitmap_count(b))) { return true; }
Nobody calls the function like this currently, but we neither prohibit or cope with this behavior. I decided to make the function cope with it. Signed-off-by: John Snow <jsnow@redhat.com> --- util/hbitmap.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)