[libvirt Patch v2 0/4] fix re-attach of bridge device for <interface type='bridge'>
It was late (early) when I sent this the first time, and the final patch (which was just a cleanup to use g_autoptr in a few places not even needed for the declared reason for the patches) wasn't compiling correctly. I intended to send the 1st and 2nd patches on the branch and forget about the 3rd (g_autoptr) until later, but instead accidentally sent patches 2 & 3. jtomko pointed out wh patch 3 hadn't worked properly, so I've updated that, added a simple patch to eliminate a superfluous cleanup: label, and am re-sending with with what *should have been* patch 1 included for the first time. If that's all too complicated: * Patch 1 was accidentally not sent in V1 * Patch 2 was acked (twice even! :-)) and hasn't changed * Patch 3 just adds g_autoptr(virConnect) in libxl_domain.c (now that jtomko pointed out the #include of datatypes.h was missing), but otherwise unchanged * Patch 4 is trivial but new. Laine Stump (4): conf: make virDomainNetNotifyActualDevice() callable for all interface types call virDomainNetNotifyActualDevice() for all interface types use g_autoptr for all virConnectPtrs used with virGetConnectNetwork() libxl: remove a now-unnecessary ret variable and cleanup: label. src/conf/domain_conf.c | 10 +++++----- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 18 +++++++----------- src/libxl/libxl_driver.c | 24 +++++++++--------------- src/lxc/lxc_driver.c | 17 +++++------------ src/lxc/lxc_process.c | 19 +++++++------------ src/qemu/qemu_process.c | 9 ++++----- 7 files changed, 38 insertions(+), 61 deletions(-) -- 2.29.2
The bridge reattach functionality in this function should be called for interface types other than just type='network', so make it callable for any type - it just becomes a NOP for types where no action is needed. In the case of <interface type='network'> we need to create a port in the network driver, and for both type='network and type='bridge' we need to reattach the bridge device (note that virDomainNetGetActualBridgeName() gets the bridge name from the appropriate (and different!) location for either type of interface). All other interfaces currently require no action. modifying callers of this function to actually call it for all interface types is in the next patch. For now the behavior should be identical pre and post-patch. (NB: the conn argument can now legitimately be NULL, so we need to change the ATTRIBUTE_NONNULL() directive for the function's declaration - I noticed when making this change that argument 3 (the NetDefPtr) could never be NULL, so I added ATTRIBUTE_NONNULL(3) while removing ATTRIBUTE_NONNULL(1) (conn)). Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 7 ++++--- src/conf/domain_conf.h | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 472ce79e67..40ca20be33 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -30933,8 +30933,9 @@ virDomainNetNotifyActualDevice(virConnectPtr conn, { virDomainNetType actualType = virDomainNetGetActualType(iface); - if (virDomainNetCreatePort(conn, dom, iface, - VIR_NETWORK_PORT_CREATE_RECLAIM) < 0) { + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && conn + && virDomainNetCreatePort(conn, dom, iface, + VIR_NETWORK_PORT_CREATE_RECLAIM) < 0) { return; } @@ -30946,7 +30947,7 @@ virDomainNetNotifyActualDevice(virConnectPtr conn, * (final arg to virNetDevTapReattachBridge()) */ ignore_value(virNetDevTapReattachBridge(iface->ifname, - iface->data.network.actual->data.bridge.brname, + virDomainNetGetActualBridgeName(iface), &iface->mac, dom->uuid, virDomainNetGetActualVirtPortProfile(iface), virDomainNetGetActualVlan(iface), diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 59bf8a12b3..ec43bbe186 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3875,7 +3875,7 @@ void virDomainNetNotifyActualDevice(virConnectPtr conn, virDomainDefPtr dom, virDomainNetDefPtr iface) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int virDomainNetReleaseActualDevice(virConnectPtr conn, -- 2.29.2
Now that this function can be called regardless of interface type (and whether or not we have a conn for the network driver), let's actually call it for all interface types. This will assure that we re-connect any disconnected bridge devices for <interface type='bridge'> as mentioned in https://bugzilla.redhat.com/show_bug.cgi?id=1730084#c26 (until now we've only been reconnecting bridge devices for <interface type='network'>) Signed-off-by: Laine Stump <laine@redhat.com> --- src/libxl/libxl_driver.c | 9 ++++----- src/lxc/lxc_process.c | 9 ++++----- src/qemu/qemu_process.c | 9 ++++----- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d58af1a869..40d8c3d174 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -367,11 +367,10 @@ libxlReconnectNotifyNets(virDomainDefPtr def) if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) virNetDevReserveName(net->ifname); - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (!conn && !(conn = virGetConnectNetwork())) - continue; - virDomainNetNotifyActualDevice(conn, def, net); - } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && !conn) + conn = virGetConnectNetwork(); + + virDomainNetNotifyActualDevice(conn, def, net); } virObjectUnref(conn); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 2266e2cc40..0b6895bbd4 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1663,11 +1663,10 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) break; } - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (!conn && !(conn = virGetConnectNetwork())) - continue; - virDomainNetNotifyActualDevice(conn, def, net); - } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && !conn) + conn = virGetConnectNetwork(); + + virDomainNetNotifyActualDevice(conn, def, net); } virObjectUnref(conn); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index f0a698ba3d..202d867289 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3411,11 +3411,10 @@ qemuProcessNotifyNets(virDomainDefPtr def) break; } - if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - if (!conn && !(conn = virGetConnectNetwork())) - continue; - virDomainNetNotifyActualDevice(conn, def, net); - } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && !conn) + conn = virGetConnectNetwork(); + + virDomainNetNotifyActualDevice(conn, def, net); } } -- 2.29.2
Signed-off-by: Laine Stump <laine@redhat.com> Reviewed-by: Ján Tomko <jtomko@redhat.com> Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/domain_conf.c | 3 +-- src/libxl/libxl_domain.c | 7 +++---- src/libxl/libxl_driver.c | 15 +++++---------- src/lxc/lxc_driver.c | 17 +++++------------ src/lxc/lxc_process.c | 10 +++------- 5 files changed, 17 insertions(+), 35 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40ca20be33..349fc28c2a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -31040,7 +31040,7 @@ virDomainNetBandwidthUpdate(virDomainNetDefPtr iface, virNetworkPortPtr port = NULL; virTypedParameterPtr params = NULL; int nparams = 0; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; int ret = -1; if (!(conn = virGetConnectNetwork())) @@ -31060,7 +31060,6 @@ virDomainNetBandwidthUpdate(virDomainNetDefPtr iface, ret = 0; cleanup: - virObjectUnref(conn); virTypedParamsFree(params, nparams); virObjectUnref(port); virObjectUnref(net); diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index ab838b317c..c046fcb3f7 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -25,6 +25,7 @@ #include "libxl_domain.h" #include "libxl_capabilities.h" +#include "datatypes.h" #include "viralloc.h" #include "virfile.h" #include "virerror.h" @@ -849,7 +850,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, char *file; virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr; unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; #ifdef LIBXL_HAVE_PVUSB hostdev_flags |= VIR_HOSTDEV_SP_USB; @@ -936,7 +937,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, } virDomainObjRemoveTransientDef(vm); - virObjectUnref(conn); } /* @@ -1050,7 +1050,7 @@ static int libxlNetworkPrepareDevices(virDomainDefPtr def) { size_t i; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; int ret = -1; for (i = 0; i < def->nnets; i++) { @@ -1096,7 +1096,6 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) ret = 0; cleanup: - virObjectUnref(conn); return ret; } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 40d8c3d174..5bd3614e21 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -355,7 +355,7 @@ static void libxlReconnectNotifyNets(virDomainDefPtr def) { size_t i; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -372,8 +372,6 @@ libxlReconnectNotifyNets(virDomainDefPtr def) virDomainNetNotifyActualDevice(conn, def, net); } - - virObjectUnref(conn); } @@ -3403,7 +3401,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, libxl_device_nic nic; int ret = -1; char mac[VIR_MAC_STRING_BUFLEN]; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; virErrorPtr save_err = NULL; libxl_device_nic_init(&nic); @@ -3478,7 +3476,6 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver, if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK && conn) virDomainNetReleaseActualDevice(conn, vm->def, net); } - virObjectUnref(conn); virObjectUnref(cfg); virErrorRestore(&save_err); return ret; @@ -3904,13 +3901,11 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver, libxl_device_nic_dispose(&nic); if (!ret) { if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - virConnectPtr conn = virGetConnectNetwork(); - if (conn) { + g_autoptr(virConnect) conn = virGetConnectNetwork(); + if (conn) virDomainNetReleaseActualDevice(conn, vm->def, detach); - virObjectUnref(conn); - } else { + else VIR_WARN("Unable to release network device '%s'", NULLSTR(detach->ifname)); - } } virDomainNetRemove(vm->def, detachidx); } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 88d3890de7..4416acf923 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3463,14 +3463,9 @@ lxcDomainAttachDeviceNetLive(virLXCDriverPtr driver, * to the one defined in the network definition. */ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - virConnectPtr netconn = virGetConnectNetwork(); - if (!netconn) + g_autoptr(virConnect) netconn = virGetConnectNetwork(); + if (!netconn || virDomainNetAllocateActualDevice(netconn, vm->def, net) < 0) return -1; - if (virDomainNetAllocateActualDevice(netconn, vm->def, net) < 0) { - virObjectUnref(netconn); - return -1; - } - virObjectUnref(netconn); } /* final validation now that actual type is known */ @@ -4028,13 +4023,11 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, if (!ret) { virErrorPreserveLast(&save_err); if (detach->type == VIR_DOMAIN_NET_TYPE_NETWORK) { - virConnectPtr conn = virGetConnectNetwork(); - if (conn) { + g_autoptr(virConnect) conn = virGetConnectNetwork(); + if (conn) virDomainNetReleaseActualDevice(conn, vm->def, detach); - virObjectUnref(conn); - } else { + else VIR_WARN("Unable to release network device '%s'", NULLSTR(detach->ifname)); - } } virDomainNetRemove(vm->def, detachidx); virDomainNetDefFree(detach); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0b6895bbd4..cbc04a3dcd 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -172,7 +172,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, virLXCDomainObjPrivatePtr priv = vm->privateData; const virNetDevVPortProfile *vport = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; VIR_DEBUG("Cleanup VM name=%s pid=%d reason=%d flags=0x%x", vm->def->name, (int)vm->pid, (int)reason, flags); @@ -281,7 +281,6 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, virDomainObjRemoveTransientDef(vm); virObjectUnref(cfg); - virObjectUnref(conn); } @@ -571,7 +570,7 @@ virLXCProcessSetupInterfaces(virLXCDriverPtr driver, size_t niface = 0; virDomainNetDefPtr net; virDomainNetType type; - virConnectPtr netconn = NULL; + g_autoptr(virConnect) netconn = NULL; virErrorPtr save_err = NULL; *veths = g_new0(char *, def->nnets + 1); @@ -680,7 +679,6 @@ virLXCProcessSetupInterfaces(virLXCDriverPtr driver, } virErrorRestore(&save_err); } - virObjectUnref(netconn); return ret; } @@ -1634,7 +1632,7 @@ static void virLXCProcessReconnectNotifyNets(virDomainDefPtr def) { size_t i; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -1668,8 +1666,6 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) virDomainNetNotifyActualDevice(conn, def, net); } - - virObjectUnref(conn); } -- 2.29.2
Signed-off-by: Laine Stump <laine@redhat.com> --- src/libxl/libxl_domain.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c046fcb3f7..afa21bf02e 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -1051,7 +1051,6 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) { size_t i; g_autoptr(virConnect) conn = NULL; - int ret = -1; for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -1063,9 +1062,9 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) */ if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) - goto cleanup; + return -1; if (virDomainNetAllocateActualDevice(conn, def, net) < 0) - goto cleanup; + return -1; } /* final validation now that actual type is known */ @@ -1090,13 +1089,11 @@ libxlNetworkPrepareDevices(virDomainDefPtr def) pcisrc->backend = VIR_DOMAIN_HOSTDEV_PCI_BACKEND_XEN; if (virDomainHostdevInsert(def, hostdev) < 0) - goto cleanup; + return -1; } } - ret = 0; - cleanup: - return ret; + return 0; } static void -- 2.29.2
On 1/8/21 4:38 PM, Laine Stump wrote:
It was late (early) when I sent this the first time, and the final patch (which was just a cleanup to use g_autoptr in a few places not even needed for the declared reason for the patches) wasn't compiling correctly. I intended to send the 1st and 2nd patches on the branch and forget about the 3rd (g_autoptr) until later, but instead accidentally sent patches 2 & 3.
jtomko pointed out wh patch 3 hadn't worked properly, so I've updated that, added a simple patch to eliminate a superfluous cleanup: label, and am re-sending with with what *should have been* patch 1 included for the first time.
If that's all too complicated:
* Patch 1 was accidentally not sent in V1
* Patch 2 was acked (twice even! :-)) and hasn't changed
* Patch 3 just adds g_autoptr(virConnect) in libxl_domain.c (now that jtomko pointed out the #include of datatypes.h was missing), but otherwise unchanged
* Patch 4 is trivial but new.
Laine Stump (4): conf: make virDomainNetNotifyActualDevice() callable for all interface types call virDomainNetNotifyActualDevice() for all interface types use g_autoptr for all virConnectPtrs used with virGetConnectNetwork() libxl: remove a now-unnecessary ret variable and cleanup: label.
src/conf/domain_conf.c | 10 +++++----- src/conf/domain_conf.h | 2 +- src/libxl/libxl_domain.c | 18 +++++++----------- src/libxl/libxl_driver.c | 24 +++++++++--------------- src/lxc/lxc_driver.c | 17 +++++------------ src/lxc/lxc_process.c | 19 +++++++------------ src/qemu/qemu_process.c | 9 ++++----- 7 files changed, 38 insertions(+), 61 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Laine Stump -
Michal Privoznik