"Michael S. Tsirkin" <m...@redhat.com> writes: > On Wed, Jun 15, 2016 at 10:41:50PM +0200, Markus Armbruster wrote: >> Range represents a range as follows. Member @start is the inclusive >> lower bound, member @end is the exclusive upper bound. Zero @end is >> special: if @start is also zero, the range is empty, else @end is to >> be interpreted as 2^64. No other empty ranges may occur. >> >> The range [0,2^64-1] cannot be represented. If you try to create it >> with range_set_bounds1(), you get the empty range instead. If you try >> to create it with range_set_bounds() or range_extend(), assertions >> fail. Before range_set_bounds() existed, the open-coded creation >> usually got you the empty range instead. Open deathtrap. >> >> Moreover, the code dealing with the janus-faced @end is too clever by >> half. >> >> Dumb this down to a more pedestrian representation: members @lob and >> @upb are inclusive lower and upper bounds. The empty range is encoded >> as @lob = 1, @upb = 0. >> >> Signed-off-by: Markus Armbruster <arm...@redhat.com> > > And now we can create the range [0,2^64-1] without issues. Nice! > Add a test for that then?
Sure! >> --- >> include/qemu/range.h | 55 >> +++++++++++++++++++++++++--------------------------- >> util/range.c | 13 +++---------- >> 2 files changed, 29 insertions(+), 39 deletions(-) >> >> diff --git a/include/qemu/range.h b/include/qemu/range.h >> index c8c46a9..06ff361 100644 >> --- a/include/qemu/range.h >> +++ b/include/qemu/range.h >> @@ -26,37 +26,37 @@ >> /* >> * Operations on 64 bit address ranges. >> * Notes: >> - * - ranges must not wrap around 0, but can include the last byte ~0x0LL. >> - * - this can not represent a full 0 to ~0x0LL range. >> + * - Ranges must not wrap around 0, but can include UINT64_MAX. >> */ >> >> -/* A structure representing a range of addresses. */ >> struct Range { >> - uint64_t begin; /* First byte of the range, or 0 if empty. */ >> - uint64_t end; /* 1 + the last byte. 0 if range empty or ends at >> ~0x0LL. */ >> + /* >> + * A non-empty range has @lob <= @upb. >> + * An empty range has @lob == @upb + 1. >> + */ >> + uint64_t lob; /* inclusive lower bound */ >> + uint64_t upb; /* inclusive upper bound */ >> }; >> >> static inline void range_invariant(Range *range) >> { >> - assert((!range->begin && !range->end) /* empty */ >> - || range->begin <= range->end - 1); /* non-empty */ >> + assert(range->lob <= range->upb || range->lob == range->upb + 1); >> } >> >> /* Compound literal encoding the empty range */ >> -#define range_empty ((Range){ .begin = 0, .end = 0 }) >> +#define range_empty ((Range){ .lob = 1, .upb = 0 }) >> >> /* Is @range empty? */ >> static inline bool range_is_empty(Range *range) >> { >> range_invariant(range); >> - return !range->begin && !range->end; >> + return range->lob > range->upb; >> } >> >> /* Does @range contain @val? */ >> static inline bool range_contains(Range *range, uint64_t val) >> { >> - return !range_is_empty(range) >> - && val >= range->begin && val <= range->end - 1; >> + return val >= range->lob && val <= range->upb; >> } >> >> /* Initialize @range to the empty range */ >> @@ -71,14 +71,11 @@ static inline void range_make_empty(Range *range) >> * Both bounds are inclusive. >> * The interval must not be empty, i.e. @lob must be less than or >> * equal @upb. >> - * The interval must not be [0,UINT64_MAX], because Range can't >> - * represent that. >> */ >> static inline void range_set_bounds(Range *range, uint64_t lob, uint64_t >> upb) >> { >> - assert(lob <= upb); >> - range->begin = lob; >> - range->end = upb + 1; /* may wrap to zero, that's okay */ >> + range->lob = lob; >> + range->upb = upb; >> assert(!range_is_empty(range)); >> } >> >> @@ -91,8 +88,12 @@ static inline void range_set_bounds(Range *range, >> uint64_t lob, uint64_t upb) >> static inline void range_set_bounds1(Range *range, >> uint64_t lob, uint64_t upb_plus1) >> { >> - range->begin = lob; >> - range->end = upb_plus1; >> + if (!lob && !upb_plus1) { >> + *range = range_empty; >> + } else { >> + range->lob = lob; >> + range->upb = upb_plus1 - 1; >> + } >> range_invariant(range); >> } >> >> @@ -100,20 +101,18 @@ static inline void range_set_bounds1(Range *range, >> static inline uint64_t range_lob(Range *range) >> { >> assert(!range_is_empty(range)); >> - return range->begin; >> + return range->lob; >> } >> >> /* Return @range's upper bound. @range must not be empty. */ >> static inline uint64_t range_upb(Range *range) >> { >> assert(!range_is_empty(range)); >> - return range->end - 1; >> + return range->upb; >> } >> >> /* >> * Extend @range to the smallest interval that includes @extend_by, too. >> - * This must not extend @range to cover the interval [0,UINT64_MAX], >> - * because Range can't represent that. >> */ >> static inline void range_extend(Range *range, Range *extend_by) >> { >> @@ -124,15 +123,13 @@ static inline void range_extend(Range *range, Range >> *extend_by) >> *range = *extend_by; >> return; >> } >> - if (range->begin > extend_by->begin) { >> - range->begin = extend_by->begin; >> + if (range->lob > extend_by->lob) { >> + range->lob = extend_by->lob; >> } >> - /* Compare last byte in case region ends at ~0x0LL */ >> - if (range->end - 1 < extend_by->end - 1) { >> - range->end = extend_by->end; >> + if (range->upb < extend_by->upb) { >> + range->upb = extend_by->upb; >> } >> - /* Must not extend to { .begin = 0, .end = 0 }: */ >> - assert(!range_is_empty(range)); >> + range_invariant(range); >> } >> >> /* Get last byte of a range from offset + length. >> diff --git a/util/range.c b/util/range.c >> index ca149a0..8359066 100644 >> --- a/util/range.c >> +++ b/util/range.c >> @@ -21,21 +21,14 @@ >> #include "qemu/osdep.h" >> #include "qemu/range.h" >> >> -/* >> - * Operations on 64 bit address ranges. >> - * Notes: >> - * - ranges must not wrap around 0, but can include the last byte ~0x0LL. >> - * - this can not represent a full 0 to ~0x0LL range. >> - */ >> - >> /* Return -1 if @a < @b, 1 if greater, and 0 if they touch or overlap. */ >> static inline int range_compare(Range *a, Range *b) >> { >> - /* Zero a->end is 2**64, and therefore not less than any b->begin */ >> - if (a->end && a->end < b->begin) { >> + /* Careful, avoid wraparound */ >> + if (b->lob && b->lob - 1 > a->upb) { >> return -1; >> } >> - if (b->end && a->begin > b->end) { >> + if (a->lob && a->lob - 1 > b->upb) { >> return 1; >> } >> return 0; > > It looks like previously, an empty range was considered > overlapping any other range: a->begin and a->end are 0. > > After this change, IIUC it is smaller than any other range, > which seems a bit arbitrary (why not greater?), > except for another empty range, for which it is > considered overlapping. > > I think it's unlikely to break anything but might be > worth changing to match previous semantics. It can't break anything because the only caller asserts "no empty ranges". I'll explain this in the commit message. I can also make function comment cover empty ranges explicitly. Would "unspecified result" be okay with you?