On Thu, Feb 02, 2023 at 04:18:04PM +0100, Peter Krempa wrote: You can initialise the variable right away here instead of ... ... making this look like lisp ;) in first 4 patches. But I get it's a subjective preference.

This also prevents a potential memleak when multiple elements would be present. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/network_conf.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b98ae6aa3f..e33925f857 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -453,7 +453,7 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, virNetworkDHCPRangeDef *range) { virSocketAddrRange *addr = &range->addr; - xmlNodePtr cur = node->children; + xmlNodePtr lease; g_autofree char *start = NULL; g_autofree char *end = NULL; @@ -480,15 +480,9 @@ virNetworkDHCPRangeDefParseXML(const char *networkName, virNetworkIPDefPrefix(ipdef)) < 0) return -1; - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE && - virXMLNodeNameEqual(cur, "lease")) { - - if (virNetworkDHCPLeaseTimeDefParseXML(&range->lease, cur) < 0) - return -1; - } - cur = cur->next; - } + if ((lease = virXMLNodeGetSubelement(node, "lease")) && + virNetworkDHCPLeaseTimeDefParseXML(&range->lease, lease) < 0) + return -1; return 0; } -- 2.39.1

The virBitmapIsBitSet API is a permissive one which returns false when the bit is not set or is out of range. We can do the same if the bitmap is NULL to aid certain situations when this can happen, but we don't want to add extra checks. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/virbitmap.c | 4 ++-- src/util/virbitmap.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 5b9204cbd7..bdcbd0aece 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -216,7 +216,7 @@ virBitmapIsSet(virBitmap *bitmap, size_t b) /** * virBitmapIsBitSet: - * @bitmap: Pointer to bitmap + * @bitmap: Pointer to bitmap (May be NULL) * @b: bit position to get * * Get setting of bit position @b in @bitmap. @@ -228,7 +228,7 @@ bool virBitmapIsBitSet(virBitmap *bitmap, size_t b) { - if (bitmap->nbits <= b) + if (!bitmap || bitmap->nbits <= b) return false; return virBitmapIsSet(bitmap, b); diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index e2314904b0..dbd88c9bb5 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -61,7 +61,7 @@ void virBitmapClearBitExpand(virBitmap *bitmap, size_t b) * Get bit @b in @bitmap. Returns false if b is out of range. */ bool virBitmapIsBitSet(virBitmap *bitmap, size_t b) - ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + G_GNUC_WARN_UNUSED_RESULT; /* * Get setting of bit position @b in @bitmap and store in @result */ -- 2.39.1

Refactor the code to use a bitmap with an enum to avoid ugly XML parser. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/domain_conf.c | 97 ++++++++++++++++++------------------- src/conf/domain_conf.h | 31 ++++-------- src/conf/virconftypes.h | 2 - src/hypervisor/virhostdev.c | 25 ++++++---- 4 files changed, 72 insertions(+), 83 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6e33a4472f..b21efeacf9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1061,6 +1061,14 @@ VIR_ENUM_IMPL(virDomainHostdevSubsysSCSIProtocol, "iscsi", ); + +VIR_ENUM_IMPL(virDomainHostdevPCIOrigstate, + VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_LAST, + "unbind", + "removeslot", + "reprobe", +); + VIR_ENUM_IMPL(virDomainHostdevSubsysUSBGuestReset, VIR_DOMAIN_HOSTDEV_USB_GUEST_RESET_LAST, "default", @@ -3365,8 +3373,10 @@ void virDomainHostdevDefClear(virDomainHostdevDef *def) case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST: VIR_FREE(def->source.subsys.u.scsi_host.wwpn); break; - case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: + virBitmapFree(def->source.subsys.u.pci.origstates); + break; + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB: case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV: case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST: break; @@ -5731,41 +5741,6 @@ virDomainHostdevSubsysUSBDefParseXML(xmlNodePtr node, return 0; } -/* The internal XML for host PCI device's original states: - * - * <origstates> - * <unbind/> - * <removeslot/> - * <reprobe/> - * </origstates> - */ -static int -virDomainHostdevSubsysPCIOrigStatesDefParseXML(xmlNodePtr node, - virDomainHostdevOrigStates *def) -{ - xmlNodePtr cur; - cur = node->children; - - while (cur != NULL) { - if (cur->type == XML_ELEMENT_NODE) { - if (virXMLNodeNameEqual(cur, "unbind")) { - def->states.pci.unbind_from_stub = true; - } else if (virXMLNodeNameEqual(cur, "removeslot")) { - def->states.pci.remove_slot = true; - } else if (virXMLNodeNameEqual(cur, "reprobe")) { - def->states.pci.reprobe = true; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported element '%s' of 'origstates'"), - cur->name); - return -1; - } - } - cur = cur->next; - } - - return 0; -} static int virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, @@ -5774,7 +5749,6 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, unsigned int flags) { xmlNodePtr address = NULL; - xmlNodePtr origstates = NULL; VIR_XPATH_NODE_AUTORESTORE(ctxt) ctxt->node = node; @@ -5788,10 +5762,35 @@ virDomainHostdevSubsysPCIDefParseXML(xmlNodePtr node, virPCIDeviceAddressParseXML(address, &def->source.subsys.u.pci.addr) < 0) return -1; - if ((flags & VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES) && - (origstates = virXPathNode("./origstates", ctxt)) && - virDomainHostdevSubsysPCIOrigStatesDefParseXML(origstates, &def->origstates) < 0) - return -1; + if ((flags & VIR_DOMAIN_DEF_PARSE_PCI_ORIG_STATES)) { + virDomainHostdevSubsysPCI *pcisrc = &def->source.subsys.u.pci; + g_autofree xmlNodePtr *nodes = NULL; + ssize_t nnodes; + size_t i; + + if ((nnodes = virXPathNodeSet("./origstates/*", ctxt, &nodes)) < 0) + return -1; + + if (nnodes > 0) { + if (!pcisrc->origstates) + pcisrc->origstates = virBitmapNew(VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_LAST); + else + virBitmapClearAll(pcisrc->origstates); + + for (i = 0; i < nnodes; i++) { + int state; + + if ((state = virDomainHostdevPCIOrigstateTypeFromString((const char *) nodes[i]->name)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unsupported element '%s' of 'origstates'"), + (const char *) nodes[i]->name); + return -1; + } + + virBitmapSetBitExpand(pcisrc->origstates, state); + } + } + } return 0; } @@ -23154,7 +23153,6 @@ virDomainHostdevDefFormatSubsysPCI(virBuffer *buf, { g_auto(virBuffer) sourceAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) sourceChildBuf = VIR_BUFFER_INIT_CHILD(buf); - g_auto(virBuffer) origstatesChildBuf = VIR_BUFFER_INIT_CHILD(&sourceChildBuf); virDomainHostdevSubsysPCI *pcisrc = &def->source.subsys.u.pci; if (def->writeFiltering != VIR_TRISTATE_BOOL_ABSENT) @@ -23176,15 +23174,14 @@ virDomainHostdevDefFormatSubsysPCI(virBuffer *buf, virPCIDeviceAddressFormat(&sourceChildBuf, pcisrc->addr, includeTypeInAddr); - if ((flags & VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES)) { - if (def->origstates.states.pci.unbind_from_stub) - virBufferAddLit(&origstatesChildBuf, "<unbind/>\n"); - - if (def->origstates.states.pci.remove_slot) - virBufferAddLit(&origstatesChildBuf, "<removeslot/>\n"); + if (pcisrc->origstates && + (flags & VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES)) { + g_auto(virBuffer) origstatesChildBuf = VIR_BUFFER_INIT_CHILD(&sourceChildBuf); + ssize_t n = -1; - if (def->origstates.states.pci.reprobe) - virBufferAddLit(&origstatesChildBuf, "<reprobe/>\n"); + while ((n = virBitmapNextSetBit(pcisrc->origstates, n)) >= 0) + virBufferAsprintf(&origstatesChildBuf, "<%s/>\n", + virDomainHostdevPCIOrigstateTypeToString(n)); virXMLFormatElement(&sourceChildBuf, "origstates", NULL, &origstatesChildBuf); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1a80399c9c..0bec072478 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -167,28 +167,14 @@ typedef enum { } virDomainHyperVMode; VIR_ENUM_DECL(virDomainHyperVMode); -struct _virDomainHostdevOrigStates { - union { - struct { - /* Does the device need to unbind from stub when - * reattaching to host? - */ - bool unbind_from_stub; - - /* Does it need to use remove_slot when reattaching - * the device to host? - */ - bool remove_slot; +typedef enum { + VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_UNBIND = 0, /* device needs to unbind from stub when reattaching */ + VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REMOVESLOT, /* use remove_slot when reattaching */ + VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REPROBE, /* reprobe driver for device when reattaching */ - /* Does it need to reprobe driver for the device when - * reattaching to host? - */ - bool reprobe; - } pci; - - /* Perhaps 'usb' in future */ - } states; -}; + VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_LAST +} virDomainHostdevPCIOrigstate; +VIR_ENUM_DECL(virDomainHostdevPCIOrigstate); struct _virDomainLeaseDef { char *lockspace; @@ -262,6 +248,8 @@ struct _virDomainHostdevSubsysUSB { struct _virDomainHostdevSubsysPCI { virPCIDeviceAddress addr; /* host address */ virDomainHostdevSubsysPCIBackendType backend; + + virBitmap *origstates; }; struct _virDomainHostdevSubsysSCSIHost { @@ -394,7 +382,6 @@ struct _virDomainHostdevDef { virDomainHostdevSubsys subsys; virDomainHostdevCaps caps; } source; - virDomainHostdevOrigStates origstates; virDomainNetTeamingInfo *teaming; virDomainDeviceInfo *info; /* Guest address */ }; diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h index d03d1d132e..e07f967814 100644 --- a/src/conf/virconftypes.h +++ b/src/conf/virconftypes.h @@ -116,8 +116,6 @@ typedef struct _virDomainHostdevCaps virDomainHostdevCaps; typedef struct _virDomainHostdevDef virDomainHostdevDef; -typedef struct _virDomainHostdevOrigStates virDomainHostdevOrigStates; - typedef struct _virDomainHostdevSubsys virDomainHostdevSubsys; typedef struct _virDomainHostdevSubsysMediatedDev virDomainHostdevSubsysMediatedDev; diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c index c0ce867596..bc5a20d691 100644 --- a/src/hypervisor/virhostdev.c +++ b/src/hypervisor/virhostdev.c @@ -873,12 +873,18 @@ virHostdevPreparePCIDevicesImpl(virHostdevManager *mgr, if (actual) { VIR_DEBUG("Saving network configuration of PCI device %s", virPCIDeviceGetName(actual)); - hostdev->origstates.states.pci.unbind_from_stub = - virPCIDeviceGetUnbindFromStub(actual); - hostdev->origstates.states.pci.remove_slot = - virPCIDeviceGetRemoveSlot(actual); - hostdev->origstates.states.pci.reprobe = - virPCIDeviceGetReprobe(actual); + + if (!pcisrc->origstates) + pcisrc->origstates = virBitmapNew(VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_LAST); + else + virBitmapClearAll(pcisrc->origstates); + + if (virPCIDeviceGetUnbindFromStub(actual)) + virBitmapSetBitExpand(pcisrc->origstates, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_UNBIND); + if (virPCIDeviceGetRemoveSlot(actual)) + virBitmapSetBitExpand(pcisrc->origstates, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REMOVESLOT); + if (virPCIDeviceGetReprobe(actual)) + virBitmapSetBitExpand(pcisrc->origstates, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REPROBE); } } @@ -1132,6 +1138,7 @@ virHostdevUpdateActivePCIDevices(virHostdevManager *mgr, for (i = 0; i < nhostdevs; i++) { const virDomainHostdevDef *hostdev = hostdevs[i]; g_autoptr(virPCIDevice) actual = NULL; + virBitmap *orig = hostdev->source.subsys.u.pci.origstates; if (virHostdevGetPCIHostDevice(hostdev, &actual) < 0) goto cleanup; @@ -1143,9 +1150,9 @@ virHostdevUpdateActivePCIDevices(virHostdevManager *mgr, goto cleanup; /* Setup the original states for the PCI device */ - virPCIDeviceSetUnbindFromStub(actual, hostdev->origstates.states.pci.unbind_from_stub); - virPCIDeviceSetRemoveSlot(actual, hostdev->origstates.states.pci.remove_slot); - virPCIDeviceSetReprobe(actual, hostdev->origstates.states.pci.reprobe); + virPCIDeviceSetUnbindFromStub(actual, virBitmapIsBitSet(orig, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_UNBIND)); + virPCIDeviceSetRemoveSlot(actual, virBitmapIsBitSet(orig, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REMOVESLOT)); + virPCIDeviceSetReprobe(actual, virBitmapIsBitSet(orig, VIR_DOMAIN_HOSTDEV_PCI_ORIGSTATE_REPROBE)); if (virPCIDeviceListAdd(mgr->activePCIHostdevs, actual) < 0) goto cleanup; -- 2.39.1