Re: [libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del

On 2/11/19 10:40 AM, Michal Privoznik wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1624204 > > The guestfwd channels are -netdevs really. Hotunplug them as > such. Also, DEVICE_DELETED event is not triggered (surprisingly, > since we're not issuing device_del rather than netdev_del) and > associated chardev is removed automagically too. This means that > we need to do qemuDomainRemoveChrDevice() minus monitor call to > remove the chardev. > > Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; > --- > src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 15 deletions(-) > So while, yes this does work and fix the issue... It's still not going to be able to remove any guestfwd that is already assigned to a guest because it'll have that "user-" prefix... It will work once the guest is restarted of course... so... > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index a0ccc3b82c..107d0fb7a9 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, > static int > qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, > virDomainObjPtr vm, > - virDomainChrDefPtr chr) > + virDomainChrDefPtr chr, > + bool monitor) > { > virObjectEventPtr event; > char *charAlias = NULL; > qemuDomainObjPrivatePtr priv = vm->privateData; > int ret = -1; > - int rc; > + int rc = 0; > > VIR_DEBUG("Removing character device %s from domain %p %s", > chr->info.alias, vm, vm->def->name); > > - if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) > - goto cleanup; > + if (monitor) { > + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) > + goto cleanup; > > - qemuDomainObjEnterMonitor(driver, vm); > - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); > + qemuDomainObjEnterMonitor(driver, vm); > + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); > > - if (qemuDomainObjExitMonitor(driver, vm) < 0) > - goto cleanup; > + if (qemuDomainObjExitMonitor(driver, vm) < 0) > + goto cleanup; > + } > > if (rc == 0 && > qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) > @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, > break; > > case VIR_DOMAIN_DEVICE_CHR: > - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); > + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); > break; > case VIR_DOMAIN_DEVICE_RNG: > ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); > @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, > virDomainDefPtr vmdef = vm->def; > virDomainChrDefPtr tmpChr; > char *devstr = NULL; > + bool guestfwd = false; > > if (!(tmpChr = virDomainChrFind(vmdef, chr))) { > virReportError(VIR_ERR_DEVICE_MISSING, > @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, > goto cleanup; > } > > + /* guestfwd channels are not really -device rather than > + * -netdev. We need to treat them slightly differently. */ > + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && > + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; > + > if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) > goto cleanup; > > @@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, > if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) > goto cleanup; Bright idea... The returned @devstr contains the "id=" value used to generate the command line, thus why not extract out the id= value to be the alias used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?

That way patch2 becomes unnecessary (I tried it). Of course fear always strikes my soul when "assuming" anything, so I figured I'd run it past you first for your thoughts. I am OK with the patches as they are except for the tiny gotcha of never being able to remove that user-channel# device.

> > - if (!async) > + if (!async && !guestfwd) > qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); > > qemuDomainObjEnterMonitor(driver, vm); > - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { > - ignore_value(qemuDomainObjExitMonitor(driver, vm)); > - goto cleanup; Hmm... If one considers that if @devstr were NULL, then this code would do nothing, then what is it's purpose? There is no other consumer of qemuBuildChrDeviceStr that could believe @devstr == NULL and ret == 0 that I could find.