6:59 p.m.
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
6:59 p.m.
New subject: [libvirt] [PATCH 1/2] conf: take advantage of VIR_AUTO* in virDomainNetCreatePort()
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(a)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
6:59 p.m.
New subject: [libvirt] [PATCH 2/2] conf: refresh network ports missing from network driver on restart
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(a)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
8:08 p.m.
New subject: [libvirt] [PATCH 1.5/3] conf: use VIR_AUTOPTR as much as possible for virNetworkPortDefPtr
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(a)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
1:52 a.m.
New subject: [libvirt] [PATCH 1.5/3] conf: use VIR_AUTOPTR as much as possible for virNetworkPortDefPtr
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(a)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
10:48 a.m.
New subject: [libvirt] [PATCH 1.5/3] conf: use VIR_AUTOPTR as much as possible for virNetworkPortDefPtr
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(a)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!
12:09 p.m.
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(-)
1:52 a.m.
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(a)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
1884
days inactive
1889
days old
7 comments
2 participants
participants (2)
-
Laine Stump -
Michal Privoznik