[libvirt] [PATCH 0/2] conf: refresh network ports missing from network driver
Patch 2/2 is the actual fix. 1/2 is just to make the fix simpler. NB: these two patches should also be included with the other patches for https://bugzilla.redhat.com/1745815 Laine Stump (2): conf: take advantage of VIR_AUTO* in virDomainNetCreatePort() conf: refresh network ports missing from network driver on restart src/conf/domain_conf.c | 82 +++++++++++++++++++----------------- src/conf/virnetworkportdef.h | 1 + 2 files changed, 44 insertions(+), 39 deletions(-) -- 2.21.0
Before adding new code to this function that will be made easier by using auto-cleaning pointers, update it to use auto-cleaning pointers (and the more modern virErrorPreserveLast()). Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 53 ++++++++++++++---------------------- src/conf/virnetworkportdef.h | 1 + 2 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 76aaa63f57..b6fa802523 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30873,60 +30873,47 @@ virDomainNetCreatePort(virConnectPtr conn, virDomainNetDefPtr iface, unsigned int flags) { - virNetworkPtr net = NULL; - int ret = -1; - virNetworkPortDefPtr portdef = NULL; - virNetworkPortPtr port = NULL; - char *portxml = NULL; - virErrorPtr saved; + virErrorPtr save_err; + VIR_AUTOUNREF(virNetworkPtr) net = NULL; + VIR_AUTOPTR(virNetworkPortDef) portdef = NULL; + VIR_AUTOUNREF(virNetworkPortPtr) port = NULL; + VIR_AUTOFREE(char *) portxml = NULL; if (!(net = virNetworkLookupByName(conn, iface->data.network.name))) return -1; if (flags & VIR_NETWORK_PORT_CREATE_RECLAIM) { if (!(portdef = virDomainNetDefActualToNetworkPort(dom, iface))) - goto cleanup; + return -1; } else { if (!(portdef = virDomainNetDefToNetworkPort(dom, iface))) - goto cleanup; + return -1; } if (!(portxml = virNetworkPortDefFormat(portdef))) - goto cleanup; + return -1; + /* prepare to re-use portdef */ virNetworkPortDefFree(portdef); portdef = NULL; if (!(port = virNetworkPortCreateXML(net, portxml, flags))) - goto cleanup; + return -1; + /* prepare to re-use portxml */ VIR_FREE(portxml); - if (!(portxml = virNetworkPortGetXMLDesc(port, 0))) - goto deleteport; - - if (!(portdef = virNetworkPortDefParseString(portxml))) - goto deleteport; - - if (virDomainNetDefActualFromNetworkPort(iface, portdef) < 0) - goto deleteport; + if (!(portxml = virNetworkPortGetXMLDesc(port, 0)) || + !(portdef = virNetworkPortDefParseString(portxml)) || + virDomainNetDefActualFromNetworkPort(iface, portdef) < 0) { + virErrorPreserveLast(&save_err); + virNetworkPortDelete(port, 0); + virErrorRestore(&save_err); + return -1; + } virNetworkPortGetUUID(port, iface->data.network.portid); - - ret = 0; - cleanup: - virNetworkPortDefFree(portdef); - VIR_FREE(portxml); - virObjectUnref(port); - virObjectUnref(net); - return ret; - - deleteport: - saved = virSaveLastError(); - virNetworkPortDelete(port, 0); - virSetError(saved); - virFreeError(saved); - goto cleanup; + return 0; } int diff --git a/src/conf/virnetworkportdef.h b/src/conf/virnetworkportdef.h index 3d42b9b6a2..796e269fe0 100644 --- a/src/conf/virnetworkportdef.h +++ b/src/conf/virnetworkportdef.h @@ -82,6 +82,7 @@ struct _virNetworkPortDef { void virNetworkPortDefFree(virNetworkPortDefPtr port); +VIR_DEFINE_AUTOPTR_FUNC(virNetworkPortDef, virNetworkPortDefFree); virNetworkPortDefPtr virNetworkPortDefParseNode(xmlDocPtr xml, -- 2.21.0
Before the refactoring that properly separated the network driver from the hypervisor driver and forced all interaction to go through public APIs, all network usage counters were zeroed when the network driver was initialized, and the network driver's now-deprecated "semi-private" API networkNotifyActualDevice() was called for every interface of every domain as each hypervisor "reconnected" its domains during a libvirtd restart, and this would refresh the usage count for each network. Post-driver-split, during libvirtd restart/reconnection of the running domains, the function virDomainNetNotifyActualDevice() is called by each hypervisor driver for every interface of every domain restart, and this function has code to re-register interfaces, but it only calls into the network driver to re-register those ports that don't already have a valid portid (ie. one that is not simply all 0), assuming that those with valid portids are already known (and counted) by the network driver. commit 7ab9bdd47 recently modified the network driver so that, in most cases, it properly resyncs each network's connection count during libvirtd (or maybe virtnetworkd) restart by iterating through the network's port list. This doesn't account for the case where a network is destroyed and restarted while there are running domains that have active ports on the network. In that case, the entire port list and connection count for that network is lost, and now even a restart of libvirtd/virtnetworkd/virtqemud, which in the past would resync the connection count, doesn't help (the network driver thinks there are no active ports, while the hypervisor driver knows about all the active ports, but mistakenly believes that the network driver also knows). The solution to this is to not just bypass valid portids during the call to virDomainNetworkNotifyActualDevice(). Instead, we query the network driver about the portid that was preserved in the domain status, and if it is not registered, we register it. (NB: while it would technically be correct to just generate a new portid for these cases, it makes for less churn in portids (and thus may make troubleshooting simpler) if we make the small fix to virDomainNetDefActualToNetworkPort() that preserves existing valid portids rather than unconditionally generating a new one.) Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b6fa802523..d1e7ac84e8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30749,8 +30749,9 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, if (VIR_ALLOC(port) < 0) return NULL; - /* Bad - we need to preserve original port uuid */ - if (virUUIDGenerate(port->uuid) < 0) { + if (virUUIDIsValid(iface->data.network.portid)) { + memcpy(port->uuid, iface->data.network.portid, VIR_UUID_BUFLEN); + } else if (virUUIDGenerate(port->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); goto error; @@ -30883,6 +30884,23 @@ virDomainNetCreatePort(virConnectPtr conn, return -1; if (flags & VIR_NETWORK_PORT_CREATE_RECLAIM) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char macstr[VIR_MAC_STRING_BUFLEN]; + + virUUIDFormat(iface->data.network.portid, uuidstr); + virMacAddrFormat(&iface->mac, macstr); + + /* if the port is already registered, then we are done */ + if (virUUIDIsValid(iface->data.network.portid) && + (port = virNetworkPortLookupByUUID(net, iface->data.network.portid))) { + VIR_DEBUG("network: %s domain: %s mac: %s port: %s - already registered, skipping", + iface->data.network.name, dom->name, macstr, uuidstr); + return 0; + } + + /* otherwise we need to create a new port */ + VIR_DEBUG("network: %s domain: %s mac: %s port: %s - not found, reclaiming", + iface->data.network.name, dom->name, macstr, uuidstr); if (!(portdef = virDomainNetDefActualToNetworkPort(dom, iface))) return -1; } else { @@ -30931,10 +30949,9 @@ virDomainNetNotifyActualDevice(virConnectPtr conn, { virDomainNetType actualType = virDomainNetGetActualType(iface); - if (!virUUIDIsValid(iface->data.network.portid)) { - if (virDomainNetCreatePort(conn, dom, iface, - VIR_NETWORK_PORT_CREATE_RECLAIM) < 0) - return; + if (virDomainNetCreatePort(conn, dom, iface, + VIR_NETWORK_PORT_CREATE_RECLAIM) < 0) { + return; } if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK || -- 2.21.0
Since the VIR_DEFINE_AUTOPTR_FUNC() was added for virNetworkPortDefPtr, I decided to convert all uses of virNetworkPortDefPtr that were appropriate to use VIR_AUTOPTR. This could be squashed into patch 1/2, or left separate, or just completely dropped. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 58 ++++++++++++++----------------- src/conf/virnetworkobj.c | 3 +- src/conf/virnetworkportdef.c | 52 +++++++++++++-------------- tests/virnetworkportxml2xmltest.c | 3 +- 4 files changed, 53 insertions(+), 63 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d1e7ac84e8..d638c455d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30565,7 +30565,8 @@ virNetworkPortDefPtr virDomainNetDefToNetworkPort(virDomainDefPtr dom, virDomainNetDefPtr iface) { - virNetworkPortDefPtr port; + VIR_AUTOPTR(virNetworkPortDef) port = NULL; + virNetworkPortDefPtr portret = NULL; if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -30580,34 +30581,31 @@ virDomainNetDefToNetworkPort(virDomainDefPtr dom, if (virUUIDGenerate(port->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); - goto error; + return NULL; } memcpy(port->owneruuid, dom->uuid, VIR_UUID_BUFLEN); if (VIR_STRDUP(port->ownername, dom->name) < 0) - goto error; + return NULL; if (VIR_STRDUP(port->group, iface->data.network.portgroup) < 0) - goto error; + return NULL; memcpy(&port->mac, &iface->mac, VIR_MAC_BUFLEN); if (virNetDevVPortProfileCopy(&port->virtPortProfile, iface->virtPortProfile) < 0) - goto error; + return NULL; if (virNetDevBandwidthCopy(&port->bandwidth, iface->bandwidth) < 0) - goto error; + return NULL; if (virNetDevVlanCopy(&port->vlan, &iface->vlan) < 0) - goto error; + return NULL; port->trustGuestRxFilters = iface->trustGuestRxFilters; - return port; - - error: - virNetworkPortDefFree(port); - return NULL; + VIR_STEAL_PTR(portret, port); + return portret; } int @@ -30728,7 +30726,8 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, virDomainNetDefPtr iface) { virDomainActualNetDefPtr actual; - virNetworkPortDefPtr port; + VIR_AUTOPTR(virNetworkPortDef) port = NULL; + virNetworkPortDefPtr portret = NULL; if (!iface->data.network.actual) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -30754,15 +30753,15 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, } else if (virUUIDGenerate(port->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to generate UUID")); - goto error; + return NULL; } memcpy(port->owneruuid, dom->uuid, VIR_UUID_BUFLEN); if (VIR_STRDUP(port->ownername, dom->name) < 0) - goto error; + return NULL; if (VIR_STRDUP(port->group, iface->data.network.portgroup) < 0) - goto error; + return NULL; memcpy(&port->mac, &iface->mac, VIR_MAC_BUFLEN); @@ -30771,7 +30770,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, port->plugtype = VIR_NETWORK_PORT_PLUG_TYPE_NETWORK; if (VIR_STRDUP(port->plug.bridge.brname, actual->data.bridge.brname) < 0) - goto error; + return NULL; port->plug.bridge.macTableManager = actual->data.bridge.macTableManager; break; @@ -30779,7 +30778,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, port->plugtype = VIR_NETWORK_PORT_PLUG_TYPE_BRIDGE; if (VIR_STRDUP(port->plug.bridge.brname, actual->data.bridge.brname) < 0) - goto error; + return NULL; port->plug.bridge.macTableManager = actual->data.bridge.macTableManager; break; @@ -30787,7 +30786,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, port->plugtype = VIR_NETWORK_PORT_PLUG_TYPE_DIRECT; if (VIR_STRDUP(port->plug.direct.linkdev, actual->data.direct.linkdev) < 0) - goto error; + return NULL; port->plug.direct.mode = actual->data.direct.mode; break; @@ -30798,7 +30797,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, _("Actual interface '%s' hostdev was not a PCI device"), iface->ifname); - goto error; + return NULL; } port->plug.hostdevpci.managed = virTristateBoolFromBool(actual->data.hostdev.def.managed); port->plug.hostdevpci.addr = actual->data.hostdev.def.source.subsys.u.pci.addr; @@ -30824,7 +30823,7 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, default: virReportEnumRangeError(virDomainHostdevSubsysPCIBackendType, actual->data.hostdev.def.source.subsys.u.pci.backend); - goto error; + return NULL; } break; @@ -30840,31 +30839,28 @@ virDomainNetDefActualToNetworkPort(virDomainDefPtr dom, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Unexpected network port type %s"), virDomainNetTypeToString(virDomainNetGetActualType(iface))); - goto error; + return NULL; case VIR_DOMAIN_NET_TYPE_LAST: default: virReportEnumRangeError(virNetworkPortPlugType, port->plugtype); - goto error; + return NULL; } if (virNetDevVPortProfileCopy(&port->virtPortProfile, actual->virtPortProfile) < 0) - goto error; + return NULL; if (virNetDevBandwidthCopy(&port->bandwidth, actual->bandwidth) < 0) - goto error; + return NULL; if (virNetDevVlanCopy(&port->vlan, &actual->vlan) < 0) - goto error; + return NULL; port->class_id = actual->class_id; port->trustGuestRxFilters = actual->trustGuestRxFilters; - return port; - - error: - virNetworkPortDefFree(port); - return NULL; + VIR_STEAL_PTR(portret, port); + return portret; } diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index ca1d598cf9..69a962b466 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1887,7 +1887,7 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, int ret = -1; int rc; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virNetworkPortDefPtr portdef = NULL; + VIR_AUTOPTR(virNetworkPortDef) portdef = NULL; if (!(dir = virNetworkObjGetPortStatusDir(net, stateDir))) goto cleanup; @@ -1925,6 +1925,5 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, ret = 0; cleanup: VIR_DIR_CLOSE(dh); - virNetworkPortDefFree(portdef); return ret; } diff --git a/src/conf/virnetworkportdef.c b/src/conf/virnetworkportdef.c index 2e20bff66e..36e2c2c444 100644 --- a/src/conf/virnetworkportdef.c +++ b/src/conf/virnetworkportdef.c @@ -75,7 +75,8 @@ virNetworkPortDefFree(virNetworkPortDefPtr def) static virNetworkPortDefPtr virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) { - virNetworkPortDefPtr def; + VIR_AUTOPTR(virNetworkPortDef) def = NULL; + virNetworkPortDefPtr defret = NULL; VIR_AUTOFREE(char *) uuid = NULL; xmlNodePtr virtPortNode; xmlNodePtr vlanNode; @@ -96,19 +97,19 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (!uuid) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network port has no uuid")); - goto error; + return NULL; } if (virUUIDParse(uuid, def->uuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse UUID '%s'"), uuid); - goto error; + return NULL; } def->ownername = virXPathString("string(./owner/name)", ctxt); if (!def->ownername) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network port has no owner name")); - goto error; + return NULL; } VIR_FREE(uuid); @@ -116,13 +117,13 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (!uuid) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network port has no owner UUID")); - goto error; + return NULL; } if (virUUIDParse(uuid, def->owneruuid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse UUID '%s'"), uuid); - goto error; + return NULL; } def->group = virXPathString("string(./group)", ctxt); @@ -130,19 +131,19 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virtPortNode = virXPathNode("./virtualport", ctxt); if (virtPortNode && (!(def->virtPortProfile = virNetDevVPortProfileParse(virtPortNode, 0)))) { - goto error; + return NULL; } mac = virXPathString("string(./mac/@address)", ctxt); if (!mac) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("network port has no mac")); - goto error; + return NULL; } if (virMacAddrParse(mac, &def->mac) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse MAC '%s'"), mac); - goto error; + return NULL; } bandwidthNode = virXPathNode("./bandwidth", ctxt); @@ -155,11 +156,11 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (bandwidthNode && virNetDevBandwidthParse(&def->bandwidth, &def->class_id, bandwidthNode, true) < 0) - goto error; + return NULL; vlanNode = virXPathNode("./vlan", ctxt); if (vlanNode && virNetDevVlanParse(vlanNode, ctxt, &def->vlan) < 0) - goto error; + return NULL; trustGuestRxFilters @@ -170,7 +171,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virReportError(VIR_ERR_XML_ERROR, _("Invalid guest rx filters trust setting '%s' "), trustGuestRxFilters); - goto error; + return NULL; } } @@ -191,7 +192,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (!(def->plug.bridge.brname = virXPathString("string(./plug/@bridge)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing network port bridge name")); - goto error; + return NULL; } macmgr = virXPathString("string(./plug/@macTableManager)", ctxt); if (macmgr && @@ -200,7 +201,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virReportError(VIR_ERR_XML_ERROR, _("Invalid macTableManager setting '%s' " "in network port"), macmgr); - goto error; + return NULL; } break; @@ -208,7 +209,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) if (!(def->plug.direct.linkdev = virXPathString("string(./plug/@dev)", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing network port link device name")); - goto error; + return NULL; } mode = virXPathString("string(./plug/@mode)", ctxt); if (mode && @@ -216,7 +217,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virNetDevMacVLanModeTypeFromString(mode)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid mode setting '%s' in network port"), mode); - goto error; + return NULL; } break; @@ -227,7 +228,7 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virTristateBoolTypeFromString(managed)) < 0) { virReportError(VIR_ERR_XML_ERROR, _("Invalid managed setting '%s' in network port"), mode); - goto error; + return NULL; } driver = virXPathString("string(./plug/driver/@name)", ctxt); if (driver && @@ -235,31 +236,26 @@ virNetworkPortDefParseXML(xmlXPathContextPtr ctxt) virNetworkForwardDriverNameTypeFromString(driver)) <= 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing network port driver name")); - goto error; + return NULL; } if (!(addressNode = virXPathNode("./plug/address", ctxt))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing network port PCI address")); - goto error; + return NULL; } if (virPCIDeviceAddressParseXML(addressNode, &def->plug.hostdevpci.addr) < 0) - goto error; + return NULL; break; case VIR_NETWORK_PORT_PLUG_TYPE_LAST: default: virReportEnumRangeError(virNetworkPortPlugType, def->plugtype); - goto error; + return NULL; } - cleanup: - return def; - - error: - virNetworkPortDefFree(def); - def = NULL; - goto cleanup; + VIR_STEAL_PTR(defret, def); + return defret; } diff --git a/tests/virnetworkportxml2xmltest.c b/tests/virnetworkportxml2xmltest.c index bb0ae8a8d5..9cbb327d92 100644 --- a/tests/virnetworkportxml2xmltest.c +++ b/tests/virnetworkportxml2xmltest.c @@ -38,7 +38,7 @@ testCompareXMLToXMLFiles(const char *expected) { char *actual = NULL; int ret = -1; - virNetworkPortDefPtr dev = NULL; + VIR_AUTOPTR(virNetworkPortDef) dev = NULL; if (!(dev = virNetworkPortDefParseFile(expected))) goto cleanup; @@ -52,7 +52,6 @@ testCompareXMLToXMLFiles(const char *expected) ret = 0; cleanup: VIR_FREE(actual); - virNetworkPortDefFree(dev); return ret; } -- 2.21.0
On 9/23/19 3:08 AM, Laine Stump wrote:
Since the VIR_DEFINE_AUTOPTR_FUNC() was added for virNetworkPortDefPtr, I decided to convert all uses of virNetworkPortDefPtr that were appropriate to use VIR_AUTOPTR. This could be squashed into patch 1/2, or left separate, or just completely dropped.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 58 ++++++++++++++----------------- src/conf/virnetworkobj.c | 3 +- src/conf/virnetworkportdef.c | 52 +++++++++++++-------------- tests/virnetworkportxml2xmltest.c | 3 +- 4 files changed, 53 insertions(+), 63 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d1e7ac84e8..d638c455d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30565,7 +30565,8 @@ virNetworkPortDefPtr virDomainNetDefToNetworkPort(virDomainDefPtr dom, virDomainNetDefPtr iface) { - virNetworkPortDefPtr port; + VIR_AUTOPTR(virNetworkPortDef) port = NULL; + virNetworkPortDefPtr portret = NULL;
Here and in the rest of the patch you don need to introduce XXXret variable, because ...
- return port; - - error: - virNetworkPortDefFree(port); - return NULL; + VIR_STEAL_PTR(portret, port); + return portret;
.. you can just use VIR_RETURN_PTR(port);
}
Also, there is one more occurrence of virNetworkPortDefFree() in networkPortCreateXML() in src/network/bridge_driver.c. In fact, code inspection says that virNetworkPortDef might be leaked there (for instance if checks involving @portdef in the middle of the function fail), so please squash this in: diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c index c54be96407..f9ef7eeb6f 100644 --- i/src/network/bridge_driver.c +++ w/src/network/bridge_driver.c @@ -5571,7 +5571,7 @@ networkPortCreateXML(virNetworkPtr net, virNetworkDriverStatePtr driver = networkGetDriver(); virNetworkObjPtr obj; virNetworkDefPtr def; - virNetworkPortDefPtr portdef = NULL; + VIR_AUTOPTR(virNetworkPortDef) portdef = NULL; virNetworkPortPtr ret = NULL; int rc; @@ -5621,13 +5621,13 @@ networkPortCreateXML(virNetworkPtr net, virErrorPreserveLast(&save_err); ignore_value(networkReleasePort(obj, portdef)); - virNetworkPortDefFree(portdef); virErrorRestore(&save_err); goto cleanup; } ret = virGetNetworkPort(net, portdef->uuid); + portdef = NULL; cleanup: virNetworkObjEndAPI(&obj); return ret; Michal
On 9/26/19 2:52 AM, Michal Privoznik wrote:
On 9/23/19 3:08 AM, Laine Stump wrote:
Since the VIR_DEFINE_AUTOPTR_FUNC() was added for virNetworkPortDefPtr, I decided to convert all uses of virNetworkPortDefPtr that were appropriate to use VIR_AUTOPTR. This could be squashed into patch 1/2, or left separate, or just completely dropped.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 58 ++++++++++++++----------------- src/conf/virnetworkobj.c | 3 +- src/conf/virnetworkportdef.c | 52 +++++++++++++-------------- tests/virnetworkportxml2xmltest.c | 3 +- 4 files changed, 53 insertions(+), 63 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d1e7ac84e8..d638c455d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30565,7 +30565,8 @@ virNetworkPortDefPtr virDomainNetDefToNetworkPort(virDomainDefPtr dom, virDomainNetDefPtr iface) { - virNetworkPortDefPtr port; + VIR_AUTOPTR(virNetworkPortDef) port = NULL; + virNetworkPortDefPtr portret = NULL;
Here and in the rest of the patch you don need to introduce XXXret variable, because ...
- return port; - - error: - virNetworkPortDefFree(port); - return NULL; + VIR_STEAL_PTR(portret, port); + return portret;
.. you can just use VIR_RETURN_PTR(port);
Ah, I never noticed that macro. Exactly what I wanted :-)
}
Also, there is one more occurrence of virNetworkPortDefFree() in networkPortCreateXML() in src/network/bridge_driver.c. In fact, code inspection says that virNetworkPortDef might be leaked there (for instance if checks involving @portdef in the middle of the function fail), so please squash this in:
diff --git i/src/network/bridge_driver.c w/src/network/bridge_driver.c index c54be96407..f9ef7eeb6f 100644 --- i/src/network/bridge_driver.c +++ w/src/network/bridge_driver.c @@ -5571,7 +5571,7 @@ networkPortCreateXML(virNetworkPtr net, virNetworkDriverStatePtr driver = networkGetDriver(); virNetworkObjPtr obj; virNetworkDefPtr def; - virNetworkPortDefPtr portdef = NULL; + VIR_AUTOPTR(virNetworkPortDef) portdef = NULL; virNetworkPortPtr ret = NULL; int rc;
@@ -5621,13 +5621,13 @@ networkPortCreateXML(virNetworkPtr net,
virErrorPreserveLast(&save_err); ignore_value(networkReleasePort(obj, portdef)); - virNetworkPortDefFree(portdef); virErrorRestore(&save_err);
goto cleanup; }
ret = virGetNetworkPort(net, portdef->uuid); + portdef = NULL; cleanup: virNetworkObjEndAPI(&obj); return ret;
I had looked at that one (I used cscope to look at all of them, and for some reason had decided it was inappropriate to convert. But your suggestion shows that I was just looking at it wrong. So yes, that looks like a good idea. I'll make the changes before pushing. Thanks!
ping On 9/21/19 7:59 PM, Laine Stump wrote:
Patch 2/2 is the actual fix. 1/2 is just to make the fix simpler.
NB: these two patches should also be included with the other patches for https://bugzilla.redhat.com/1745815
Laine Stump (2): conf: take advantage of VIR_AUTO* in virDomainNetCreatePort() conf: refresh network ports missing from network driver on restart
src/conf/domain_conf.c | 82 +++++++++++++++++++----------------- src/conf/virnetworkportdef.h | 1 + 2 files changed, 44 insertions(+), 39 deletions(-)
On 9/22/19 1:59 AM, Laine Stump wrote:
Patch 2/2 is the actual fix. 1/2 is just to make the fix simpler.
NB: these two patches should also be included with the other patches for https://bugzilla.redhat.com/1745815
Laine Stump (2): conf: take advantage of VIR_AUTO* in virDomainNetCreatePort() conf: refresh network ports missing from network driver on restart
src/conf/domain_conf.c | 82 +++++++++++++++++++----------------- src/conf/virnetworkportdef.h | 1 + 2 files changed, 44 insertions(+), 39 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> And I don't care if you squash 1.5/2 into 1/2 as you suggest or push it as a separate patch. But please address my comments first. Michal
participants (2)
-
Laine Stump -
Michal Privoznik