On 04/25/2017 12:33 PM, Laine Stump wrote: > Nothing that could happen during networkNotifyActualDevice() could > justify unceremoniously killing the qemu process, but that's what we > were doing. So that galaxy in far far away land circa commit id '04711a0f' when all this was added - > > In particular, new code added in commit 85bcc022 (first appearred in appeared > libvirt-3.2.0) attempts to reattach tap devices to their assigned > bridge devices when libvirtd restarts (to make it easier to recover > from a restart of a libvirt network). But if the network has been > stopped and *not* restarted, the bridge device won't exist and > networkNotifyActualDevice() will fail. > > This patch changes qemuProcessNotifyNets() to return void, so that > qemuProcessReconnect() will soldier on regardless of what happens (any > errors will still be logged, but we won't terminate the guest). > > Partially resolves: https://bugzilla.redhat.com/1442700 > --- > > NB: there are many other things that might fail during reconnection of > a qemu process that shouldn't lead to killing the qemu > process. qemuProcessReconnect() could use a good audit to change what > happens when certain items fail. > Nose game, not me ;-) > > > src/qemu/qemu_process.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 53170d7..fcb5763 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -2825,7 +2825,7 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, > > > > -static int > +static void > qemuProcessNotifyNets(virDomainDefPtr def) > { > size_t i; > @@ -2840,10 +2840,8 @@ qemuProcessNotifyNets(virDomainDefPtr def) > if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) > ignore_value(virNetDevMacVLanReserveName(net->ifname, false)); > > - if (networkNotifyActualDevice(def, net) < 0) > - return -1; > + networkNotifyActualDevice(def, net); Seems like networkNotifyActualDevice should then return a void too? And have it's comments/content changed... IIUC thought, essentially if the network goes away, the domain isn't necessarily shut down - it gets notified somehow that the network isn't there any more and soldiers on. So from that aspect I can certain agree that just because we discover an error when libvirtd restarts doesn't mean we should now kill the domain. I think the concept behind the patch is fine - it's the "how much more" should be done... (and yes with the eye on keeping backports to a minimum, yep understood). John > } > - return 0; > } > > static int > @@ -3480,8 +3478,7 @@ qemuProcessReconnect(void *opaque) > if (qemuSecurityReserveLabel(driver->securityManager, obj->def, obj->pid) < 0) > goto error; > > - if (qemuProcessNotifyNets(obj->def) < 0) > - goto error; > + qemuProcessNotifyNets(obj->def); > > if (qemuProcessFiltersInstantiate(obj->def)) > goto error; > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
