Re: [Qemu-devel] [PATCH RFC 2/4] range: Eliminate direct Range member access

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On 06/15/2016 02:41 PM, Markus Armbruster wrote:
> Users of struct Range mess liberally with its members, which makes
> refactoring hard.  Create a set of methods, and convert all users to
> call them instead of accessing members.  The methods have carefully
> worded contracts, and use assertions to check them.
> 
> To help with tracking down the places that access members of struct
> Range directly, hide the implementation of struct Range outside of
> range.c by trickery.  The trickery will be dropped in the next commit.

Can't we just make Range an opaque type, and move its definition into
range.c?  Oh, except we have inline functions that would no longer be
inline, unless we also had a range-impl.h private header.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---


> @@ -2015,7 +2017,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>                           0, 0x000A0000, 0x000BFFFF, 0, 0x00020000));
>  
>      crs_replace_with_free_ranges(mem_ranges,
> -                                 pci_hole->begin, pci_hole->end - 1);
> +                                 range_lob(pci_hole),
> +                                 range_upb(pci_hole));

Changes like this are nice, isolating us from what the actual struct stores.


> +++ b/include/qemu/range.h
> @@ -30,18 +30,110 @@
>   *   - this can not represent a full 0 to ~0x0LL range.
>   */
>  
> +bool range_is_empty(Range *range);
> +bool range_contains(Range *range, uint64_t val);
> +void range_make_empty(Range *range);
> +void range_set_bounds(Range *range, uint64_t lob, uint64_t upb);
> +void range_set_bounds1(Range *range, uint64_t lob, uint64_t upb_plus1);
> +uint64_t range_lob(Range *range);
> +uint64_t range_upb(Range *range);
> +void range_extend(Range *range, Range *extend_by);
> +#ifdef RANGE_IMPL
> +
>  /* 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. */
>  };
>  
> +static inline void range_invariant(Range *range)
> +{
> +    assert((!range->begin && !range->end) /* empty */
> +           || range->begin <= range->end - 1); /* non-empty */
> +}
> +
> +#define static
> +#define inline

Yeah, that's a bit of a hack.  I can live with it though.

The other alternative is to squash 2 and 3 into a single patch on
commit; but having them separate was a nice review aid.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]