Re: [Qemu-devel] [PATCH v1 4/7] range: add some more functions

[David Hildenbrand <david@redhat.com>]

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.


-- 

Thanks,

David / dhildenb