| Message ID | 1466023310-13221-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> writes: > 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. I'm not sure the inline functions are warranted, but I'd rather not debate that right now. So this patch makes Range kind-of opaque temporarily, by renaming its members outside range.c, just to help me find their users, and to make it more obvious to you that I found them all. >> 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. I'm quite willing to squash if my trickery is considered too gross to be preserved for posterity. My personal preference is not to squash, for a full record. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
On Wed, Jun 15, 2016 at 10:41:48PM +0200, 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. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> I guess you want me to merge this because of the changes in pc and pci? > --- > hw/i386/acpi-build.c | 35 +++++++------- > hw/pci-host/piix.c | 26 +++++++---- > hw/pci-host/q35.c | 41 +++++++++++------ > hw/pci/pci.c | 17 +++---- > include/qemu/range.h | 106 ++++++++++++++++++++++++++++++++++++++++++- > qapi/string-input-visitor.c | 20 ++++---- > qapi/string-output-visitor.c | 18 ++++---- > util/log.c | 5 +- > util/range.c | 4 +- > 9 files changed, 198 insertions(+), 74 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 02fc534..6c36c24 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -232,18 +232,20 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64) > pci_host = acpi_get_i386_pci_host(); > g_assert(pci_host); > > - hole->begin = object_property_get_int(pci_host, > - PCI_HOST_PROP_PCI_HOLE_START, > - NULL); > - hole->end = object_property_get_int(pci_host, > - PCI_HOST_PROP_PCI_HOLE_END, > - NULL); > - hole64->begin = object_property_get_int(pci_host, > - PCI_HOST_PROP_PCI_HOLE64_START, > - NULL); > - hole64->end = object_property_get_int(pci_host, > - PCI_HOST_PROP_PCI_HOLE64_END, > - NULL); > + range_set_bounds1(hole, > + object_property_get_int(pci_host, > + PCI_HOST_PROP_PCI_HOLE_START, > + NULL), > + object_property_get_int(pci_host, > + PCI_HOST_PROP_PCI_HOLE_END, > + NULL)); > + range_set_bounds1(hole64, > + object_property_get_int(pci_host, > + PCI_HOST_PROP_PCI_HOLE64_START, > + NULL), > + object_property_get_int(pci_host, > + PCI_HOST_PROP_PCI_HOLE64_END, > + NULL)); > } > > #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */ > @@ -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)); > for (i = 0; i < mem_ranges->len; i++) { > entry = g_ptr_array_index(mem_ranges, i); > aml_append(crs, > @@ -2025,12 +2028,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > 0, entry->limit - entry->base + 1)); > } > > - if (pci_hole64->begin) { > + if (!range_is_empty(pci_hole64)) { > aml_append(crs, > aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, > AML_CACHEABLE, AML_READ_WRITE, > - 0, pci_hole64->begin, pci_hole64->end - 1, 0, > - pci_hole64->end - pci_hole64->begin)); > + 0, range_lob(pci_hole64), range_upb(pci_hole64), 0, > + range_upb(pci_hole64) + 1 - range_lob(pci_hole64))); > } > > if (misc->tpm_version != TPM_VERSION_UNSPEC) { > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > index 8db0f09..1df327f 100644 > --- a/hw/pci-host/piix.c > +++ b/hw/pci-host/piix.c > @@ -221,8 +221,12 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v, > Error **errp) > { > I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); > - uint32_t value = s->pci_hole.begin; > + uint64_t val64; > + uint32_t value; > > + val64 = range_is_empty(&s->pci_hole) ? 0 : range_lob(&s->pci_hole); > + value = val64; > + assert(value == val64); > visit_type_uint32(v, name, &value, errp); > } > > @@ -231,8 +235,12 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v, > Error **errp) > { > I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); > - uint32_t value = s->pci_hole.end; > + uint64_t val64; > + uint32_t value; > > + val64 = range_is_empty(&s->pci_hole) ? 0 : range_upb(&s->pci_hole) + 1; > + value = val64; > + assert(value == val64); > visit_type_uint32(v, name, &value, errp); > } > > @@ -242,10 +250,11 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > Range w64; > + uint64_t value; > > pci_bus_get_w64_range(h->bus, &w64); > - > - visit_type_uint64(v, name, &w64.begin, errp); > + value = range_is_empty(&w64) ? 0 : range_lob(&w64); > + visit_type_uint64(v, name, &value, errp); > } > > static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, > @@ -254,10 +263,11 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > Range w64; > + uint64_t value; > > pci_bus_get_w64_range(h->bus, &w64); > - > - visit_type_uint64(v, name, &w64.end, errp); > + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; > + visit_type_uint64(v, name, &value, errp); > } > > static void i440fx_pcihost_initfn(Object *obj) > @@ -344,8 +354,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type, > f->ram_memory = ram_memory; > > i440fx = I440FX_PCI_HOST_BRIDGE(dev); > - i440fx->pci_hole.begin = below_4g_mem_size; > - i440fx->pci_hole.end = IO_APIC_DEFAULT_ADDRESS; > + range_set_bounds(&i440fx->pci_hole, below_4g_mem_size, > + IO_APIC_DEFAULT_ADDRESS - 1); > > /* setup pci memory mapping */ > pc_pci_as_mapping_init(OBJECT(f), f->system_memory, > diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c > index 8c2c1db..d200091 100644 > --- a/hw/pci-host/q35.c > +++ b/hw/pci-host/q35.c > @@ -73,8 +73,13 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v, > Error **errp) > { > Q35PCIHost *s = Q35_HOST_DEVICE(obj); > - uint32_t value = s->mch.pci_hole.begin; > + uint64_t val64; > + uint32_t value; > > + val64 = range_is_empty(&s->mch.pci_hole) > + ? 0 : range_lob(&s->mch.pci_hole); > + value = val64; > + assert(value == val64); > visit_type_uint32(v, name, &value, errp); > } > > @@ -83,8 +88,13 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v, > Error **errp) > { > Q35PCIHost *s = Q35_HOST_DEVICE(obj); > - uint32_t value = s->mch.pci_hole.end; > + uint64_t val64; > + uint32_t value; > > + val64 = range_is_empty(&s->mch.pci_hole) > + ? 0 : range_upb(&s->mch.pci_hole) + 1; > + value = val64; > + assert(value == val64); > visit_type_uint32(v, name, &value, errp); > } > > @@ -94,10 +104,11 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > Range w64; > + uint64_t value; > > pci_bus_get_w64_range(h->bus, &w64); > - > - visit_type_uint64(v, name, &w64.begin, errp); > + value = range_is_empty(&w64) ? 0 : range_lob(&w64); > + visit_type_uint64(v, name, &value, errp); > } > > static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, > @@ -106,10 +117,11 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, > { > PCIHostState *h = PCI_HOST_BRIDGE(obj); > Range w64; > + uint64_t value; > > pci_bus_get_w64_range(h->bus, &w64); > - > - visit_type_uint64(v, name, &w64.end, errp); > + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; > + visit_type_uint64(v, name, &value, errp); > } > > static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, > @@ -182,9 +194,9 @@ static void q35_host_initfn(Object *obj) > * it's not a power of two, which means an MTRR > * can't cover it exactly. > */ > - s->mch.pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + > - MCH_HOST_BRIDGE_PCIEXBAR_MAX; > - s->mch.pci_hole.end = IO_APIC_DEFAULT_ADDRESS; > + range_set_bounds(&s->mch.pci_hole, > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX, > + IO_APIC_DEFAULT_ADDRESS - 1); > } > > static const TypeInfo q35_host_info = { > @@ -252,10 +264,7 @@ static void mch_update_pciexbar(MCHPCIState *mch) > break; > case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD: > default: > - enable = 0; > - length = 0; > abort(); > - break; > } > addr = pciexbar & addr_mask; > pcie_host_mmcfg_update(pehb, enable, addr, length); > @@ -265,9 +274,13 @@ static void mch_update_pciexbar(MCHPCIState *mch) > * which means an MTRR can't cover it exactly. > */ > if (enable) { > - mch->pci_hole.begin = addr + length; > + range_set_bounds(&mch->pci_hole, > + addr + length, > + IO_APIC_DEFAULT_ADDRESS - 1); > } else { > - mch->pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; > + range_set_bounds(&mch->pci_hole, > + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, > + IO_APIC_DEFAULT_ADDRESS - 1); > } > } > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index bb605ef..904c6fd 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -2436,13 +2436,13 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) > > if (limit >= base) { > Range pref_range; > - pref_range.begin = base; > - pref_range.end = limit + 1; > + range_set_bounds(&pref_range, base, limit); > range_extend(range, &pref_range); > } > } > for (i = 0; i < PCI_NUM_REGIONS; ++i) { > PCIIORegion *r = &dev->io_regions[i]; > + pcibus_t lob, upb; > Range region_range; > > if (!r->size || > @@ -2450,16 +2450,17 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) > !(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) { > continue; > } > - region_range.begin = pci_bar_address(dev, i, r->type, r->size); > - region_range.end = region_range.begin + r->size; > > - if (region_range.begin == PCI_BAR_UNMAPPED) { > + lob = pci_bar_address(dev, i, r->type, r->size); > + upb = lob + r->size - 1; > + if (lob == PCI_BAR_UNMAPPED) { > continue; > } > > - region_range.begin = MAX(region_range.begin, 0x1ULL << 32); > + lob = MAX(lob, 0x1ULL << 32); > > - if (region_range.end - 1 >= region_range.begin) { > + if (upb >= lob) { > + range_set_bounds(®ion_range, lob, upb); > range_extend(range, ®ion_range); > } > } > @@ -2467,7 +2468,7 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) > > void pci_bus_get_w64_range(PCIBus *bus, Range *range) > { > - range->begin = range->end = 0; > + range_make_empty(range); > pci_for_each_device_under_bus(bus, pci_dev_get_w64, range); > } > > diff --git a/include/qemu/range.h b/include/qemu/range.h > index 3970f00..9296ba0 100644 > --- a/include/qemu/range.h > +++ 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 > + > +/* Compound literal encoding the empty range */ > +#define range_empty ((Range){ .begin = 0, .end = 0 }) > + > +/* Is @range empty? */ > +static inline bool range_is_empty(Range *range) > +{ > + range_invariant(range); > + return !range->begin && !range->end; > +} > + > +/* 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; > +} > + > +/* Initialize @range to the empty range */ > +static inline void range_make_empty(Range *range) > +{ > + *range = range_empty; > + assert(range_is_empty(range)); > +} > + > +/* > + * Initialize @range to span the interval [@lob,@upb]. > + * 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 */ > + assert(!range_is_empty(range)); > +} > + > +/* > + * Initialize @range to span the interval [@lob,@upb_plus1). > + * The lower bound is inclusive, the upper bound is exclusive. > + * Zero @upb_plus1 is special: if @lob is also zero, set @range to the > + * empty range. Else, set @range to [@lob,UINT64_MAX]. > + */ > +static inline void range_set_bounds1(Range *range, > + uint64_t lob, uint64_t upb_plus1) > +{ > + range->begin = lob; > + range->end = upb_plus1; > + range_invariant(range); > +} > + > +/* Return @range's lower bound. @range must not be empty. */ > +static inline uint64_t range_lob(Range *range) > +{ > + assert(!range_is_empty(range)); > + return range->begin; > +} > + > +/* 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; > +} > + > +/* > + * 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) > { > - if (!extend_by->begin && !extend_by->end) { > + if (range_is_empty(extend_by)) { > return; > } > - if (!range->begin && !range->end) { > + if (range_is_empty(range)) { > *range = *extend_by; > return; > } > @@ -52,8 +144,18 @@ static inline void range_extend(Range *range, Range *extend_by) > if (range->end - 1 < extend_by->end - 1) { > range->end = extend_by->end; > } > + /* Must not extend to { .begin = 0, .end = 0 }: */ > + assert(!range_is_empty(range)); > } > > +#undef static > +#undef inline > +#else > +struct Range { > + uint64_t begin_, end_; > +}; > +#endif > + > /* Get last byte of a range from offset + length. > * Undefined for ranges that wrap around 0. */ > static inline uint64_t range_get_last(uint64_t offset, uint64_t len) > diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c > index b546e5f..0690abb 100644 > --- a/qapi/string-input-visitor.c > +++ b/qapi/string-input-visitor.c > @@ -59,8 +59,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) > if (errno == 0 && endptr > str) { > if (*endptr == '\0') { > cur = g_malloc0(sizeof(*cur)); > - cur->begin = start; > - cur->end = start + 1; > + range_set_bounds(cur, start, start); > siv->ranges = range_list_insert(siv->ranges, cur); > cur = NULL; > str = NULL; > @@ -73,16 +72,14 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) > end < start + 65536)) { > if (*endptr == '\0') { > cur = g_malloc0(sizeof(*cur)); > - cur->begin = start; > - cur->end = end + 1; > + range_set_bounds(cur, start, end); > siv->ranges = range_list_insert(siv->ranges, cur); > cur = NULL; > str = NULL; > } else if (*endptr == ',') { > str = endptr + 1; > cur = g_malloc0(sizeof(*cur)); > - cur->begin = start; > - cur->end = end + 1; > + range_set_bounds(cur, start, end); > siv->ranges = range_list_insert(siv->ranges, cur); > cur = NULL; > } else { > @@ -94,8 +91,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) > } else if (*endptr == ',') { > str = endptr + 1; > cur = g_malloc0(sizeof(*cur)); > - cur->begin = start; > - cur->end = start + 1; > + range_set_bounds(cur, start, start); > siv->ranges = range_list_insert(siv->ranges, cur); > cur = NULL; > } else { > @@ -134,7 +130,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size, > if (siv->cur_range) { > Range *r = siv->cur_range->data; > if (r) { > - siv->cur = r->begin; > + siv->cur = range_lob(r); > } > *list = g_malloc0(size); > } else { > @@ -156,7 +152,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) > return NULL; > } > > - if (siv->cur < r->begin || siv->cur >= r->end) { > + if (!range_contains(r, siv->cur)) { > siv->cur_range = g_list_next(siv->cur_range); > if (!siv->cur_range) { > return NULL; > @@ -165,7 +161,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) > if (!r) { > return NULL; > } > - siv->cur = r->begin; > + siv->cur = range_lob(r); > } > > tail->next = g_malloc0(size); > @@ -208,7 +204,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, > goto error; > } > > - siv->cur = r->begin; > + siv->cur = range_lob(r); > } > > *obj = siv->cur; > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c > index 5ea395a..c6f01f9 100644 > --- a/qapi/string-output-visitor.c > +++ b/qapi/string-output-visitor.c > @@ -83,8 +83,8 @@ static void string_output_set(StringOutputVisitor *sov, char *string) > static void string_output_append(StringOutputVisitor *sov, int64_t a) > { > Range *r = g_malloc0(sizeof(*r)); > - r->begin = a; > - r->end = a + 1; > + > + range_set_bounds(r, a, a); > sov->ranges = range_list_insert(sov->ranges, r); > } > > @@ -92,28 +92,28 @@ static void string_output_append_range(StringOutputVisitor *sov, > int64_t s, int64_t e) > { > Range *r = g_malloc0(sizeof(*r)); > - r->begin = s; > - r->end = e + 1; > + > + range_set_bounds(r, s, e); > sov->ranges = range_list_insert(sov->ranges, r); > } > > static void format_string(StringOutputVisitor *sov, Range *r, bool next, > bool human) > { > - if (r->end - r->begin > 1) { > + if (range_lob(r) != range_upb(r)) { > if (human) { > g_string_append_printf(sov->string, "0x%" PRIx64 "-0x%" PRIx64, > - r->begin, r->end - 1); > + range_lob(r), range_upb(r)); > > } else { > g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64, > - r->begin, r->end - 1); > + range_lob(r), range_upb(r)); > } > } else { > if (human) { > - g_string_append_printf(sov->string, "0x%" PRIx64, r->begin); > + g_string_append_printf(sov->string, "0x%" PRIx64, range_lob(r)); > } else { > - g_string_append_printf(sov->string, "%" PRId64, r->begin); > + g_string_append_printf(sov->string, "%" PRId64, range_lob(r)); > } > } > if (next) { > diff --git a/util/log.c b/util/log.c > index f811d61..4da635c 100644 > --- a/util/log.c > +++ b/util/log.c > @@ -132,7 +132,7 @@ bool qemu_log_in_addr_range(uint64_t addr) > int i = 0; > for (i = 0; i < debug_regions->len; i++) { > Range *range = &g_array_index(debug_regions, Range, i); > - if (addr >= range->begin && addr <= range->end - 1) { > + if (range_contains(range, addr)) { > return true; > } > } > @@ -208,8 +208,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) > error_setg(errp, "Invalid range"); > goto out; > } > - range.begin = lob; > - range.end = upb + 1; > + range_set_bounds(&range, lob, upb); > g_array_append_val(debug_regions, range); > } > out: > diff --git a/util/range.c b/util/range.c > index 56e6baf..ab5102a 100644 > --- a/util/range.c > +++ b/util/range.c > @@ -19,6 +19,7 @@ > */ > > #include "qemu/osdep.h" > +#define RANGE_IMPL > #include "qemu/range.h" > > /* > @@ -46,8 +47,7 @@ GList *range_list_insert(GList *list, Range *data) > { > GList *l; > > - /* Range lists require no empty ranges */ > - assert(data->begin < data->end || (data->begin && !data->end)); > + assert(!range_is_empty(data)); > > for (l = list; l && range_compare(l->data, data) < 0; l = l->next) { > /* Skip all list elements strictly less than data */ > -- > 2.5.5
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Jun 15, 2016 at 10:41:48PM +0200, 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. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Thanks! > I guess you want me to merge this because of the changes in pc and pci? Yes, please (whole series, once respun without the RFC).
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 02fc534..6c36c24 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -232,18 +232,20 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64) pci_host = acpi_get_i386_pci_host(); g_assert(pci_host); - hole->begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_START, - NULL); - hole->end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE_END, - NULL); - hole64->begin = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_START, - NULL); - hole64->end = object_property_get_int(pci_host, - PCI_HOST_PROP_PCI_HOLE64_END, - NULL); + range_set_bounds1(hole, + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_START, + NULL), + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE_END, + NULL)); + range_set_bounds1(hole64, + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_START, + NULL), + object_property_get_int(pci_host, + PCI_HOST_PROP_PCI_HOLE64_END, + NULL)); } #define ACPI_PORT_SMI_CMD 0x00b2 /* TODO: this is APM_CNT_IOPORT */ @@ -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)); for (i = 0; i < mem_ranges->len; i++) { entry = g_ptr_array_index(mem_ranges, i); aml_append(crs, @@ -2025,12 +2028,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, 0, entry->limit - entry->base + 1)); } - if (pci_hole64->begin) { + if (!range_is_empty(pci_hole64)) { aml_append(crs, aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, AML_CACHEABLE, AML_READ_WRITE, - 0, pci_hole64->begin, pci_hole64->end - 1, 0, - pci_hole64->end - pci_hole64->begin)); + 0, range_lob(pci_hole64), range_upb(pci_hole64), 0, + range_upb(pci_hole64) + 1 - range_lob(pci_hole64))); } if (misc->tpm_version != TPM_VERSION_UNSPEC) { diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c index 8db0f09..1df327f 100644 --- a/hw/pci-host/piix.c +++ b/hw/pci-host/piix.c @@ -221,8 +221,12 @@ static void i440fx_pcihost_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_hole.begin; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->pci_hole) ? 0 : range_lob(&s->pci_hole); + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -231,8 +235,12 @@ static void i440fx_pcihost_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { I440FXState *s = I440FX_PCI_HOST_BRIDGE(obj); - uint32_t value = s->pci_hole.end; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->pci_hole) ? 0 : range_upb(&s->pci_hole) + 1; + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -242,10 +250,11 @@ static void i440fx_pcihost_get_pci_hole64_start(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.begin, errp); + value = range_is_empty(&w64) ? 0 : range_lob(&w64); + visit_type_uint64(v, name, &value, errp); } static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, @@ -254,10 +263,11 @@ static void i440fx_pcihost_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.end, errp); + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; + visit_type_uint64(v, name, &value, errp); } static void i440fx_pcihost_initfn(Object *obj) @@ -344,8 +354,8 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type, f->ram_memory = ram_memory; i440fx = I440FX_PCI_HOST_BRIDGE(dev); - i440fx->pci_hole.begin = below_4g_mem_size; - i440fx->pci_hole.end = IO_APIC_DEFAULT_ADDRESS; + range_set_bounds(&i440fx->pci_hole, below_4g_mem_size, + IO_APIC_DEFAULT_ADDRESS - 1); /* setup pci memory mapping */ pc_pci_as_mapping_init(OBJECT(f), f->system_memory, diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c index 8c2c1db..d200091 100644 --- a/hw/pci-host/q35.c +++ b/hw/pci-host/q35.c @@ -73,8 +73,13 @@ static void q35_host_get_pci_hole_start(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_hole.begin; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->mch.pci_hole) + ? 0 : range_lob(&s->mch.pci_hole); + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -83,8 +88,13 @@ static void q35_host_get_pci_hole_end(Object *obj, Visitor *v, Error **errp) { Q35PCIHost *s = Q35_HOST_DEVICE(obj); - uint32_t value = s->mch.pci_hole.end; + uint64_t val64; + uint32_t value; + val64 = range_is_empty(&s->mch.pci_hole) + ? 0 : range_upb(&s->mch.pci_hole) + 1; + value = val64; + assert(value == val64); visit_type_uint32(v, name, &value, errp); } @@ -94,10 +104,11 @@ static void q35_host_get_pci_hole64_start(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.begin, errp); + value = range_is_empty(&w64) ? 0 : range_lob(&w64); + visit_type_uint64(v, name, &value, errp); } static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, @@ -106,10 +117,11 @@ static void q35_host_get_pci_hole64_end(Object *obj, Visitor *v, { PCIHostState *h = PCI_HOST_BRIDGE(obj); Range w64; + uint64_t value; pci_bus_get_w64_range(h->bus, &w64); - - visit_type_uint64(v, name, &w64.end, errp); + value = range_is_empty(&w64) ? 0 : range_upb(&w64) + 1; + visit_type_uint64(v, name, &value, errp); } static void q35_host_get_mmcfg_size(Object *obj, Visitor *v, const char *name, @@ -182,9 +194,9 @@ static void q35_host_initfn(Object *obj) * it's not a power of two, which means an MTRR * can't cover it exactly. */ - s->mch.pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + - MCH_HOST_BRIDGE_PCIEXBAR_MAX; - s->mch.pci_hole.end = IO_APIC_DEFAULT_ADDRESS; + range_set_bounds(&s->mch.pci_hole, + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT + MCH_HOST_BRIDGE_PCIEXBAR_MAX, + IO_APIC_DEFAULT_ADDRESS - 1); } static const TypeInfo q35_host_info = { @@ -252,10 +264,7 @@ static void mch_update_pciexbar(MCHPCIState *mch) break; case MCH_HOST_BRIDGE_PCIEXBAR_LENGTH_RVD: default: - enable = 0; - length = 0; abort(); - break; } addr = pciexbar & addr_mask; pcie_host_mmcfg_update(pehb, enable, addr, length); @@ -265,9 +274,13 @@ static void mch_update_pciexbar(MCHPCIState *mch) * which means an MTRR can't cover it exactly. */ if (enable) { - mch->pci_hole.begin = addr + length; + range_set_bounds(&mch->pci_hole, + addr + length, + IO_APIC_DEFAULT_ADDRESS - 1); } else { - mch->pci_hole.begin = MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT; + range_set_bounds(&mch->pci_hole, + MCH_HOST_BRIDGE_PCIEXBAR_DEFAULT, + IO_APIC_DEFAULT_ADDRESS - 1); } } diff --git a/hw/pci/pci.c b/hw/pci/pci.c index bb605ef..904c6fd 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2436,13 +2436,13 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) if (limit >= base) { Range pref_range; - pref_range.begin = base; - pref_range.end = limit + 1; + range_set_bounds(&pref_range, base, limit); range_extend(range, &pref_range); } } for (i = 0; i < PCI_NUM_REGIONS; ++i) { PCIIORegion *r = &dev->io_regions[i]; + pcibus_t lob, upb; Range region_range; if (!r->size || @@ -2450,16 +2450,17 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) !(r->type & PCI_BASE_ADDRESS_MEM_TYPE_64)) { continue; } - region_range.begin = pci_bar_address(dev, i, r->type, r->size); - region_range.end = region_range.begin + r->size; - if (region_range.begin == PCI_BAR_UNMAPPED) { + lob = pci_bar_address(dev, i, r->type, r->size); + upb = lob + r->size - 1; + if (lob == PCI_BAR_UNMAPPED) { continue; } - region_range.begin = MAX(region_range.begin, 0x1ULL << 32); + lob = MAX(lob, 0x1ULL << 32); - if (region_range.end - 1 >= region_range.begin) { + if (upb >= lob) { + range_set_bounds(®ion_range, lob, upb); range_extend(range, ®ion_range); } } @@ -2467,7 +2468,7 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) void pci_bus_get_w64_range(PCIBus *bus, Range *range) { - range->begin = range->end = 0; + range_make_empty(range); pci_for_each_device_under_bus(bus, pci_dev_get_w64, range); } diff --git a/include/qemu/range.h b/include/qemu/range.h index 3970f00..9296ba0 100644 --- a/include/qemu/range.h +++ 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 + +/* Compound literal encoding the empty range */ +#define range_empty ((Range){ .begin = 0, .end = 0 }) + +/* Is @range empty? */ +static inline bool range_is_empty(Range *range) +{ + range_invariant(range); + return !range->begin && !range->end; +} + +/* 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; +} + +/* Initialize @range to the empty range */ +static inline void range_make_empty(Range *range) +{ + *range = range_empty; + assert(range_is_empty(range)); +} + +/* + * Initialize @range to span the interval [@lob,@upb]. + * 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 */ + assert(!range_is_empty(range)); +} + +/* + * Initialize @range to span the interval [@lob,@upb_plus1). + * The lower bound is inclusive, the upper bound is exclusive. + * Zero @upb_plus1 is special: if @lob is also zero, set @range to the + * empty range. Else, set @range to [@lob,UINT64_MAX]. + */ +static inline void range_set_bounds1(Range *range, + uint64_t lob, uint64_t upb_plus1) +{ + range->begin = lob; + range->end = upb_plus1; + range_invariant(range); +} + +/* Return @range's lower bound. @range must not be empty. */ +static inline uint64_t range_lob(Range *range) +{ + assert(!range_is_empty(range)); + return range->begin; +} + +/* 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; +} + +/* + * 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) { - if (!extend_by->begin && !extend_by->end) { + if (range_is_empty(extend_by)) { return; } - if (!range->begin && !range->end) { + if (range_is_empty(range)) { *range = *extend_by; return; } @@ -52,8 +144,18 @@ static inline void range_extend(Range *range, Range *extend_by) if (range->end - 1 < extend_by->end - 1) { range->end = extend_by->end; } + /* Must not extend to { .begin = 0, .end = 0 }: */ + assert(!range_is_empty(range)); } +#undef static +#undef inline +#else +struct Range { + uint64_t begin_, end_; +}; +#endif + /* Get last byte of a range from offset + length. * Undefined for ranges that wrap around 0. */ static inline uint64_t range_get_last(uint64_t offset, uint64_t len) diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c index b546e5f..0690abb 100644 --- a/qapi/string-input-visitor.c +++ b/qapi/string-input-visitor.c @@ -59,8 +59,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) if (errno == 0 && endptr > str) { if (*endptr == '\0') { cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = start + 1; + range_set_bounds(cur, start, start); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; @@ -73,16 +72,14 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) end < start + 65536)) { if (*endptr == '\0') { cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = end + 1; + range_set_bounds(cur, start, end); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; str = NULL; } else if (*endptr == ',') { str = endptr + 1; cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = end + 1; + range_set_bounds(cur, start, end); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { @@ -94,8 +91,7 @@ static int parse_str(StringInputVisitor *siv, const char *name, Error **errp) } else if (*endptr == ',') { str = endptr + 1; cur = g_malloc0(sizeof(*cur)); - cur->begin = start; - cur->end = start + 1; + range_set_bounds(cur, start, start); siv->ranges = range_list_insert(siv->ranges, cur); cur = NULL; } else { @@ -134,7 +130,7 @@ start_list(Visitor *v, const char *name, GenericList **list, size_t size, if (siv->cur_range) { Range *r = siv->cur_range->data; if (r) { - siv->cur = r->begin; + siv->cur = range_lob(r); } *list = g_malloc0(size); } else { @@ -156,7 +152,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) return NULL; } - if (siv->cur < r->begin || siv->cur >= r->end) { + if (!range_contains(r, siv->cur)) { siv->cur_range = g_list_next(siv->cur_range); if (!siv->cur_range) { return NULL; @@ -165,7 +161,7 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) if (!r) { return NULL; } - siv->cur = r->begin; + siv->cur = range_lob(r); } tail->next = g_malloc0(size); @@ -208,7 +204,7 @@ static void parse_type_int64(Visitor *v, const char *name, int64_t *obj, goto error; } - siv->cur = r->begin; + siv->cur = range_lob(r); } *obj = siv->cur; diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c index 5ea395a..c6f01f9 100644 --- a/qapi/string-output-visitor.c +++ b/qapi/string-output-visitor.c @@ -83,8 +83,8 @@ static void string_output_set(StringOutputVisitor *sov, char *string) static void string_output_append(StringOutputVisitor *sov, int64_t a) { Range *r = g_malloc0(sizeof(*r)); - r->begin = a; - r->end = a + 1; + + range_set_bounds(r, a, a); sov->ranges = range_list_insert(sov->ranges, r); } @@ -92,28 +92,28 @@ static void string_output_append_range(StringOutputVisitor *sov, int64_t s, int64_t e) { Range *r = g_malloc0(sizeof(*r)); - r->begin = s; - r->end = e + 1; + + range_set_bounds(r, s, e); sov->ranges = range_list_insert(sov->ranges, r); } static void format_string(StringOutputVisitor *sov, Range *r, bool next, bool human) { - if (r->end - r->begin > 1) { + if (range_lob(r) != range_upb(r)) { if (human) { g_string_append_printf(sov->string, "0x%" PRIx64 "-0x%" PRIx64, - r->begin, r->end - 1); + range_lob(r), range_upb(r)); } else { g_string_append_printf(sov->string, "%" PRId64 "-%" PRId64, - r->begin, r->end - 1); + range_lob(r), range_upb(r)); } } else { if (human) { - g_string_append_printf(sov->string, "0x%" PRIx64, r->begin); + g_string_append_printf(sov->string, "0x%" PRIx64, range_lob(r)); } else { - g_string_append_printf(sov->string, "%" PRId64, r->begin); + g_string_append_printf(sov->string, "%" PRId64, range_lob(r)); } } if (next) { diff --git a/util/log.c b/util/log.c index f811d61..4da635c 100644 --- a/util/log.c +++ b/util/log.c @@ -132,7 +132,7 @@ bool qemu_log_in_addr_range(uint64_t addr) int i = 0; for (i = 0; i < debug_regions->len; i++) { Range *range = &g_array_index(debug_regions, Range, i); - if (addr >= range->begin && addr <= range->end - 1) { + if (range_contains(range, addr)) { return true; } } @@ -208,8 +208,7 @@ void qemu_set_dfilter_ranges(const char *filter_spec, Error **errp) error_setg(errp, "Invalid range"); goto out; } - range.begin = lob; - range.end = upb + 1; + range_set_bounds(&range, lob, upb); g_array_append_val(debug_regions, range); } out: diff --git a/util/range.c b/util/range.c index 56e6baf..ab5102a 100644 --- a/util/range.c +++ b/util/range.c @@ -19,6 +19,7 @@ */ #include "qemu/osdep.h" +#define RANGE_IMPL #include "qemu/range.h" /* @@ -46,8 +47,7 @@ GList *range_list_insert(GList *list, Range *data) { GList *l; - /* Range lists require no empty ranges */ - assert(data->begin < data->end || (data->begin && !data->end)); + assert(!range_is_empty(data)); for (l = list; l && range_compare(l->data, data) < 0; l = l->next) { /* Skip all list elements strictly less than data */
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. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/i386/acpi-build.c | 35 +++++++------- hw/pci-host/piix.c | 26 +++++++---- hw/pci-host/q35.c | 41 +++++++++++------ hw/pci/pci.c | 17 +++---- include/qemu/range.h | 106 ++++++++++++++++++++++++++++++++++++++++++- qapi/string-input-visitor.c | 20 ++++---- qapi/string-output-visitor.c | 18 ++++---- util/log.c | 5 +- util/range.c | 4 +- 9 files changed, 198 insertions(+), 74 deletions(-)