| Message ID | 20181009205652.10605-5-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/range/memory-device: fixes and cleanups | expand |
* David Hildenbrand (david@redhat.com) wrote: > Add some more functions that will be used in memory-device context. > > range_init(): Init using lower bound and size > range_valid(): Check if there would be an overflow when initializin > range_size(): Extract the size of a range > range_overlaps_range(): Check for overlaps of two ranges > range_contains_range(): Check if one range is contained in the other > range_starts_before_range(): Check if one range starts before another > range_ends_after_range(): Check if one range ends after another > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 80 insertions(+) > > diff --git a/include/qemu/range.h b/include/qemu/range.h > index 7e75f4e655..18e8acf22f 100644 > --- a/include/qemu/range.h > +++ b/include/qemu/range.h > @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) > return range->upb; > } > > +/* > + * Initialize @range to span the interval [@lob,@lob + @size - 1]. > + * @size may be 0. > + */ > +static inline void range_init(Range *range, uint64_t lob, uint64_t size) > +{ > + range->lob = lob; > + range->upb = lob + size - 1; > + range_invariant(range); > +} > + > +/* > + * Check if the interval [@lob,@lob + @size - 1] would be valid or not > + * (result in an overflow). > + */ > +static inline bool range_valid(uint64_t lob, uint64_t size) > +{ > + return lob + size >= lob; > +} That name confused me, I'd expected that to have taken a range and check it for something (like a non-asserting version of the invariant). > +/* > + * Get the size of @range. > + */ > +static inline uint64_t range_size(const Range *range) > +{ > + return range->upb - range->lob + 1; > +} > + > +/* > + * Check if @range1 overlaps with @range2. If one of the ranges is empty, > + * the result is always "false". > + */ > +static inline bool range_overlaps_range(const Range *range1, > + const Range *range2) > +{ > + if (range_is_empty(range1) || range_is_empty(range2)) { > + return false; > + } > + return !(range2->upb < range1->lob || range1->upb < range2->lob); > +} > + > +/* > + * Check if @range1 contains @range2. If one of the ranges is empty, > + * the result is always "false". > + */ > +static inline bool range_contains_range(const Range *range1, > + const Range *range2) > +{ > + if (range_is_empty(range1) || range_is_empty(range2)) { > + return false; > + } > + return range1->lob <= range2->lob && range1->upb >= range2->upb; > +} > + > +/* > + * Check if @range1 starts before @range2. If one of the ranges is empty, > + * the result is alsways "false". > + */ > +static inline bool range_starts_before_range(const Range *range1, > + const Range *range2) > +{ > + if (range_is_empty(range1) || range_is_empty(range2)) { > + return false; > + } > + return range1->lob < range2->lob; > +} > + > +/* > + * Check if @range1 ends after @range2. If one of the ranges is empty, > + * the result is alsways "false". > + */ > +static inline bool range_ends_after_range(const Range *range1, > + const Range *range2) > +{ > + if (range_is_empty(range1) || range_is_empty(range2)) { > + return false; > + } > + return range1->upb > range2->upb; > +} > + > /* > * Extend @range to the smallest interval that includes @extend_by, too. > */ > -- > 2.17.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/10/2018 11:08, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> Add some more functions that will be used in memory-device context. >> >> range_init(): Init using lower bound and size >> range_valid(): Check if there would be an overflow when initializin >> range_size(): Extract the size of a range >> range_overlaps_range(): Check for overlaps of two ranges >> range_contains_range(): Check if one range is contained in the other >> range_starts_before_range(): Check if one range starts before another >> range_ends_after_range(): Check if one range ends after another >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 80 insertions(+) >> >> diff --git a/include/qemu/range.h b/include/qemu/range.h >> index 7e75f4e655..18e8acf22f 100644 >> --- a/include/qemu/range.h >> +++ b/include/qemu/range.h >> @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) >> return range->upb; >> } >> >> +/* >> + * Initialize @range to span the interval [@lob,@lob + @size - 1]. >> + * @size may be 0. >> + */ >> +static inline void range_init(Range *range, uint64_t lob, uint64_t size) >> +{ >> + range->lob = lob; >> + range->upb = lob + size - 1; >> + range_invariant(range); >> +} >> + >> +/* >> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not >> + * (result in an overflow). >> + */ >> +static inline bool range_valid(uint64_t lob, uint64_t size) >> +{ >> + return lob + size >= lob; >> +} > > That name confused me, I'd expected that to have taken a range and check > it for something (like a non-asserting version of the invariant). Then we have to remove all the variant asserts from the initializer functions (well, because then it is no longer an invariant then). Other ideas?
* David Hildenbrand (david@redhat.com) wrote: > On 11/10/2018 11:08, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > >> Add some more functions that will be used in memory-device context. > >> > >> range_init(): Init using lower bound and size > >> range_valid(): Check if there would be an overflow when initializin > >> range_size(): Extract the size of a range > >> range_overlaps_range(): Check for overlaps of two ranges > >> range_contains_range(): Check if one range is contained in the other > >> range_starts_before_range(): Check if one range starts before another > >> range_ends_after_range(): Check if one range ends after another > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 80 insertions(+) > >> > >> diff --git a/include/qemu/range.h b/include/qemu/range.h > >> index 7e75f4e655..18e8acf22f 100644 > >> --- a/include/qemu/range.h > >> +++ b/include/qemu/range.h > >> @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) > >> return range->upb; > >> } > >> > >> +/* > >> + * Initialize @range to span the interval [@lob,@lob + @size - 1]. > >> + * @size may be 0. > >> + */ > >> +static inline void range_init(Range *range, uint64_t lob, uint64_t size) > >> +{ > >> + range->lob = lob; > >> + range->upb = lob + size - 1; > >> + range_invariant(range); > >> +} > >> + > >> +/* > >> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not > >> + * (result in an overflow). > >> + */ > >> +static inline bool range_valid(uint64_t lob, uint64_t size) > >> +{ > >> + return lob + size >= lob; > >> +} > > > > That name confused me, I'd expected that to have taken a range and check > > it for something (like a non-asserting version of the invariant). > > Then we have to remove all the variant asserts from the initializer > functions (well, because then it is no longer an invariant then). Other > ideas? My worry here is just the name 'range_valid'. Dave > -- > > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/10/2018 11:21, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote: >>> * David Hildenbrand (david@redhat.com) wrote: >>>> Add some more functions that will be used in memory-device context. >>>> >>>> range_init(): Init using lower bound and size >>>> range_valid(): Check if there would be an overflow when initializin >>>> range_size(): Extract the size of a range >>>> range_overlaps_range(): Check for overlaps of two ranges >>>> range_contains_range(): Check if one range is contained in the other >>>> range_starts_before_range(): Check if one range starts before another >>>> range_ends_after_range(): Check if one range ends after another >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 80 insertions(+) >>>> >>>> diff --git a/include/qemu/range.h b/include/qemu/range.h >>>> index 7e75f4e655..18e8acf22f 100644 >>>> --- a/include/qemu/range.h >>>> +++ b/include/qemu/range.h >>>> @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) >>>> return range->upb; >>>> } >>>> >>>> +/* >>>> + * Initialize @range to span the interval [@lob,@lob + @size - 1]. >>>> + * @size may be 0. >>>> + */ >>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size) >>>> +{ >>>> + range->lob = lob; >>>> + range->upb = lob + size - 1; >>>> + range_invariant(range); >>>> +} >>>> + >>>> +/* >>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not >>>> + * (result in an overflow). >>>> + */ >>>> +static inline bool range_valid(uint64_t lob, uint64_t size) >>>> +{ >>>> + return lob + size >= lob; >>>> +} >>> >>> That name confused me, I'd expected that to have taken a range and check >>> it for something (like a non-asserting version of the invariant). >> >> Then we have to remove all the variant asserts from the initializer >> functions (well, because then it is no longer an invariant then). Other >> ideas? > > My worry here is just the name 'range_valid'. > hmm "range_would_overflow()" ? > Dave
* David Hildenbrand (david@redhat.com) wrote: > On 11/10/2018 11:21, Dr. David Alan Gilbert wrote: > > * David Hildenbrand (david@redhat.com) wrote: > >> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote: > >>> * David Hildenbrand (david@redhat.com) wrote: > >>>> Add some more functions that will be used in memory-device context. > >>>> > >>>> range_init(): Init using lower bound and size > >>>> range_valid(): Check if there would be an overflow when initializin > >>>> range_size(): Extract the size of a range > >>>> range_overlaps_range(): Check for overlaps of two ranges > >>>> range_contains_range(): Check if one range is contained in the other > >>>> range_starts_before_range(): Check if one range starts before another > >>>> range_ends_after_range(): Check if one range ends after another > >>>> > >>>> Signed-off-by: David Hildenbrand <david@redhat.com> > >>>> --- > >>>> include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 80 insertions(+) > >>>> > >>>> diff --git a/include/qemu/range.h b/include/qemu/range.h > >>>> index 7e75f4e655..18e8acf22f 100644 > >>>> --- a/include/qemu/range.h > >>>> +++ b/include/qemu/range.h > >>>> @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) > >>>> return range->upb; > >>>> } > >>>> > >>>> +/* > >>>> + * Initialize @range to span the interval [@lob,@lob + @size - 1]. > >>>> + * @size may be 0. > >>>> + */ > >>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size) > >>>> +{ > >>>> + range->lob = lob; > >>>> + range->upb = lob + size - 1; > >>>> + range_invariant(range); > >>>> +} > >>>> + > >>>> +/* > >>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not > >>>> + * (result in an overflow). > >>>> + */ > >>>> +static inline bool range_valid(uint64_t lob, uint64_t size) > >>>> +{ > >>>> + return lob + size >= lob; > >>>> +} > >>> > >>> That name confused me, I'd expected that to have taken a range and check > >>> it for something (like a non-asserting version of the invariant). > >> > >> Then we have to remove all the variant asserts from the initializer > >> functions (well, because then it is no longer an invariant then). Other > >> ideas? > > > > My worry here is just the name 'range_valid'. > > > > hmm "range_would_overflow()" ? Yes, a bit long but OK. But another observation; in the following patch, you're tending to do: if (!range_valid(...)) moan range_init(...) would it make more sense to change range_init so it was: static inline bool range_init(Range *range, uint64_t lob, uint64_t size) { range->lob = lob; range->upb = lob + size - 1; return ob + size >= lob; } and then in the places you use it, you could do: if (!range_init(...) moan Dave > > Dave > > -- > > Thanks, > > David / dhildenb -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 11/10/2018 12:27, Dr. David Alan Gilbert wrote: > * David Hildenbrand (david@redhat.com) wrote: >> On 11/10/2018 11:21, Dr. David Alan Gilbert wrote: >>> * David Hildenbrand (david@redhat.com) wrote: >>>> On 11/10/2018 11:08, Dr. David Alan Gilbert wrote: >>>>> * David Hildenbrand (david@redhat.com) wrote: >>>>>> Add some more functions that will be used in memory-device context. >>>>>> >>>>>> range_init(): Init using lower bound and size >>>>>> range_valid(): Check if there would be an overflow when initializin >>>>>> range_size(): Extract the size of a range >>>>>> range_overlaps_range(): Check for overlaps of two ranges >>>>>> range_contains_range(): Check if one range is contained in the other >>>>>> range_starts_before_range(): Check if one range starts before another >>>>>> range_ends_after_range(): Check if one range ends after another >>>>>> >>>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>>> --- >>>>>> include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 80 insertions(+) >>>>>> >>>>>> diff --git a/include/qemu/range.h b/include/qemu/range.h >>>>>> index 7e75f4e655..18e8acf22f 100644 >>>>>> --- a/include/qemu/range.h >>>>>> +++ b/include/qemu/range.h >>>>>> @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) >>>>>> return range->upb; >>>>>> } >>>>>> >>>>>> +/* >>>>>> + * Initialize @range to span the interval [@lob,@lob + @size - 1]. >>>>>> + * @size may be 0. >>>>>> + */ >>>>>> +static inline void range_init(Range *range, uint64_t lob, uint64_t size) >>>>>> +{ >>>>>> + range->lob = lob; >>>>>> + range->upb = lob + size - 1; >>>>>> + range_invariant(range); >>>>>> +} >>>>>> + >>>>>> +/* >>>>>> + * Check if the interval [@lob,@lob + @size - 1] would be valid or not >>>>>> + * (result in an overflow). >>>>>> + */ >>>>>> +static inline bool range_valid(uint64_t lob, uint64_t size) >>>>>> +{ >>>>>> + return lob + size >= lob; >>>>>> +} >>>>> >>>>> That name confused me, I'd expected that to have taken a range and check >>>>> it for something (like a non-asserting version of the invariant). >>>> >>>> Then we have to remove all the variant asserts from the initializer >>>> functions (well, because then it is no longer an invariant then). Other >>>> ideas? >>> >>> My worry here is just the name 'range_valid'. >>> >> >> hmm "range_would_overflow()" ? > > Yes, a bit long but OK. > > But another observation; in the following patch, you're tending to do: > > if (!range_valid(...)) > moan > > > range_init(...) > > would it make more sense to change range_init so it was: > > static inline bool range_init(Range *range, uint64_t lob, uint64_t size) > { > range->lob = lob; > range->upb = lob + size - 1; > return ob + size >= lob; > } > > > and then in the places you use it, you could do: > > if (!range_init(...) > moan > Yes, that makes sense.
diff --git a/include/qemu/range.h b/include/qemu/range.h index 7e75f4e655..18e8acf22f 100644 --- a/include/qemu/range.h +++ b/include/qemu/range.h @@ -112,6 +112,86 @@ static inline uint64_t range_upb(Range *range) return range->upb; } +/* + * Initialize @range to span the interval [@lob,@lob + @size - 1]. + * @size may be 0. + */ +static inline void range_init(Range *range, uint64_t lob, uint64_t size) +{ + range->lob = lob; + range->upb = lob + size - 1; + range_invariant(range); +} + +/* + * Check if the interval [@lob,@lob + @size - 1] would be valid or not + * (result in an overflow). + */ +static inline bool range_valid(uint64_t lob, uint64_t size) +{ + return lob + size >= lob; +} + +/* + * Get the size of @range. + */ +static inline uint64_t range_size(const Range *range) +{ + return range->upb - range->lob + 1; +} + +/* + * Check if @range1 overlaps with @range2. If one of the ranges is empty, + * the result is always "false". + */ +static inline bool range_overlaps_range(const Range *range1, + const Range *range2) +{ + if (range_is_empty(range1) || range_is_empty(range2)) { + return false; + } + return !(range2->upb < range1->lob || range1->upb < range2->lob); +} + +/* + * Check if @range1 contains @range2. If one of the ranges is empty, + * the result is always "false". + */ +static inline bool range_contains_range(const Range *range1, + const Range *range2) +{ + if (range_is_empty(range1) || range_is_empty(range2)) { + return false; + } + return range1->lob <= range2->lob && range1->upb >= range2->upb; +} + +/* + * Check if @range1 starts before @range2. If one of the ranges is empty, + * the result is alsways "false". + */ +static inline bool range_starts_before_range(const Range *range1, + const Range *range2) +{ + if (range_is_empty(range1) || range_is_empty(range2)) { + return false; + } + return range1->lob < range2->lob; +} + +/* + * Check if @range1 ends after @range2. If one of the ranges is empty, + * the result is alsways "false". + */ +static inline bool range_ends_after_range(const Range *range1, + const Range *range2) +{ + if (range_is_empty(range1) || range_is_empty(range2)) { + return false; + } + return range1->upb > range2->upb; +} + /* * Extend @range to the smallest interval that includes @extend_by, too. */
Add some more functions that will be used in memory-device context. range_init(): Init using lower bound and size range_valid(): Check if there would be an overflow when initializin range_size(): Extract the size of a range range_overlaps_range(): Check for overlaps of two ranges range_contains_range(): Check if one range is contained in the other range_starts_before_range(): Check if one range starts before another range_ends_after_range(): Check if one range ends after another Signed-off-by: David Hildenbrand <david@redhat.com> --- include/qemu/range.h | 80 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+)