On Thu, Mar 21, 2019 at 08:49:38PM -0400, Cole Robinson wrote:
> 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
The unref is missing, but it shuoldn't be here. It need to be at
the end of the function, as we want to keep a single conn open
for the loop over all NICs.
> > 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
Yep, will add here.
> > @@ -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
I'll add it at the end.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
