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@xxxxxxxxxx>
> ---
> 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
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6ec69d3c96..a8b3ed9099 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3266,6 +3266,7 @@ static void
> qemuProcessNotifyNets(virDomainDefPtr def)
> {
> size_t i;
> + virConnectPtr conn = NULL;
>
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDefPtr net = def->nets[i];
> @@ -3277,9 +3278,14 @@ qemuProcessNotifyNets(virDomainDefPtr def)
> if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT)
> ignore_value(virNetDevMacVLanReserveName(net->ifname, false));
>
> - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK)
> - virDomainNetNotifyActualDevice(def, net);
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + if (!conn && !(conn = virGetConnectNetwork()))
> + continue;
> + virDomainNetNotifyActualDevice(conn, def, net);
> + }
> }
> +
> + virObjectUnref(conn);
> }
>
> /* Attempt to instantiate the filters. Ignore failures because it's
> @@ -5455,6 +5461,7 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
> {
> int ret = -1;
> size_t i;
> + virConnectPtr conn = NULL;
>
> for (i = 0; i < def->nnets; i++) {
> virDomainNetDefPtr net = def->nets[i];
> @@ -5464,9 +5471,12 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
> * 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(def, net) < 0)
> - goto cleanup;
> + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> + if (!conn && !(conn = virGetConnectNetwork()))
> + goto cleanup;
> + if (virDomainNetAllocateActualDevice(conn, def, net) < 0)
> + goto cleanup;
> + }
>
> actualType = virDomainNetGetActualType(net);
> if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV &&
> @@ -5497,6 +5507,7 @@ qemuProcessNetworkPrepareDevices(virDomainDefPtr def)
> }
> ret = 0;
> cleanup:
> + virObjectUnref(conn);
> return ret;
> }
>
> @@ -7111,6 +7122,7 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> size_t i;
> char *timestamp;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + virConnectPtr conn = NULL;
>
> VIR_DEBUG("Shutting down vm=%p name=%s id=%d pid=%lld, "
> "reason=%s, asyncJob=%s, flags=0x%x",
> @@ -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
Provided there's a good reason for sprinkling the conn lookup
everywhere, and with those issues fixed:
Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx>
And it could go in now. Hopefully libvirt-tck has some good device
hotplug coverage !
- Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
