Re: [libvirt] [PATCH v3 02/36] network: pass a virNetworkPtr to port management APIs

On 3/19/19 8:46 AM, Daniel P. Berrangé wrote: > The APIs for allocating/notifying/removing network ports just take > an internal domain interface struct right now. As a step towards > turning these into public facing APIs, add a virNetworkPtr argument > to all of them. > > Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > --- > src/conf/domain_conf.c | 40 ++++++++++++++++++++---- > src/conf/domain_conf.h | 18 +++++++---- > src/libxl/libxl_domain.c | 30 +++++++++++++----- > src/libxl/libxl_driver.c | 26 +++++++++++----- > src/lxc/lxc_driver.c | 24 +++++++++++--- > src/lxc/lxc_process.c | 24 +++++++++----- > src/network/bridge_driver.c | 54 ++++++++++++++++++-------------- > src/qemu/qemu_hotplug.c | 62 +++++++++++++++++++++++++++---------- > src/qemu/qemu_process.c | 30 +++++++++++++----- > 9 files changed, 223 insertions(+), 85 deletions(-) > Like I mentioned in patch #1, it seems like we could move the virConnectPtr conn = virGetConnectNetwork() into the domain_conf.c functions. virDomainNetResolveActualType in domain_conf.c already does similar. The only reason I can think of is that it saves double opening the connection in some error paths but that doesn't seem to be enough to justify the nastyness of sprinkling these calls everywhere Also I'd suggest naming the 'conn' variable consistently 'netconn' if only to make it more clear what driver we are talking about. One other side bit: virGetConnectNetwork() calls virConnectOpen() which is a public API, complete with calling ResetLastError and DispatchError. That seems wrong? Seems like it should call an internal function that doesn't skips those calls. Not the fault of this patch series though ... > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 7849aaf5b9..d9362c5ff6 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -165,6 +165,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, > virLXCDomainObjPrivatePtr priv = vm->privateData; > virNetDevVPortProfilePtr vport = NULL; > virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); > + virConnectPtr conn = NULL; > > VIR_DEBUG("Cleanup VM name=%s pid=%d reason=%d", > vm->def->name, (int)vm->pid, (int)reason); > @@ -224,8 +225,12 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, > iface->ifname)); > ignore_value(virNetDevVethDelete(iface->ifname)); > } > - if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) > - virDomainNetReleaseActualDevice(vm->def, iface); > + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (conn || (conn = virGetConnectNetwork())) > + virDomainNetReleaseActualDevice(conn, vm->def, iface); > + else > + VIR_WARN("Unable to release network device '%s'", NULLSTR(iface->ifname)); > + } > } Missing an unref here

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 97c9de04f0..becded57d9 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1374,6 +1374,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > bool charDevPlugged = false; > bool netdevPlugged = false; > char *netdev_name; > + virConnectPtr conn = NULL; > > /* preallocate new slot for device */ > if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0) > @@ -1383,9 +1384,12 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > * network's pool of devices, or resolve bridge device name > * to the one defined in the network definition. > */ > - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && > - virDomainNetAllocateActualDevice(vm->def, net) < 0) > - goto cleanup; > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (!(conn = virGetConnectNetwork())) > + goto cleanup; > + if (virDomainNetAllocateActualDevice(conn, vm->def, net) < 0) > + goto cleanup; > + } > > actualType = virDomainNetGetActualType(net); > > @@ -1688,8 +1692,12 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > > virDomainNetRemoveHostdev(vm->def, net); > > - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) > - virDomainNetReleaseActualDevice(vm->def, net); > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (conn) > + virDomainNetReleaseActualDevice(conn, vm->def, net); > + else > + VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname)); > + } > } > > VIR_FREE(nicstr); > @@ -1709,6 +1717,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > VIR_FREE(vhostfd); > VIR_FREE(vhostfdName); > VIR_FREE(charDevAlias); > + virObjectUnref(conn); > virObjectUnref(cfg); > virDomainCCWAddressSetFree(ccwaddrs); > > @@ -3719,6 +3728,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > bool needVlanUpdate = false; > int ret = -1; > int changeidx = -1; > + virConnectPtr conn = NULL; > > if ((changeidx = virDomainNetFindIdx(vm->def, newdev)) < 0) > goto cleanup; > @@ -3894,9 +3904,11 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > /* allocate new actual device to compare to old - we will need to > * free it if we fail for any reason > */ > - if (newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK && > - virDomainNetAllocateActualDevice(vm->def, newdev) < 0) { > - goto cleanup; > + if (newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (!(conn = virGetConnectNetwork())) > + goto cleanup; > + if (virDomainNetAllocateActualDevice(conn, vm->def, newdev) < 0) > + goto cleanup; > } >> newType = virDomainNetGetActualType(newdev); > @@ -4107,8 +4119,12 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > > /* this function doesn't work with HOSTDEV networks yet, thus > * no need to change the pointer in the hostdev structure */ > - if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK) > - virDomainNetReleaseActualDevice(vm->def, olddev); > + if (olddev->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (conn || (conn = virGetConnectNetwork())) > + virDomainNetReleaseActualDevice(conn, vm->def, olddev); > + else > + VIR_WARN("Unable to release network device '%s'", NULLSTR(olddev->ifname)); > + } > virDomainNetDefFree(olddev); > /* move newdev into the nets list, and NULL it out from the > * virDomainDeviceDef that we were given so that the caller > @@ -4139,8 +4155,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, > * that the changes were minor enough that we didn't need to > * replace the entire device object. > */ > - if (newdev && newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK) > - virDomainNetReleaseActualDevice(vm->def, newdev); > + if (newdev && newdev->type == VIR_DOMAIN_NET_TYPE_NETWORK && conn) > + virDomainNetReleaseActualDevice(conn, vm->def, newdev); > > return ret; > } Missing unref

> @@ -7311,8 +7323,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, > > /* kick the device out of the hostdev list too */ > virDomainNetRemoveHostdev(def, net); > - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) > - virDomainNetReleaseActualDevice(vm->def, net); > + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > + if (conn || (conn = virGetConnectNetwork())) > + virDomainNetReleaseActualDevice(conn, vm->def, net); > + else > + VIR_WARN("Unable to release network device '%s'", NULLSTR(net->ifname)); > + } > } > > retry: > Missing an unref here