Re: [libvirt] [PATCH v11 3/5] qemu: Need to remove TLS object in RemoveRNGDevice

On Tue, Oct 25, 2016 at 09:19:58AM -0400, John Ferlan wrote: > > > On 10/25/2016 08:53 AM, Pavel Hrdina wrote: >> On Mon, Oct 24, 2016 at 06:46:19PM -0400, John Ferlan wrote: >>> Commit id '6e6b4bfc' added the object, but forgot the other end. >>> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_hotplug.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> ACK if you agree with review on the first patch that the order of >> qemuMonitorDelObject and qemuMonitorDetachCharDev should be swapped. >> >> Pavel >> > > I do agree, but I hadn't fully convinced myself - I needed the extra set > of eyes to help with that... Of course, I botched that again here by > removing the tlsobj before the chardev. A problem which naturally > persists in patch 5, but is easily adjusted.. > > I looked at and thought about qemuDomainRemoveRNGDevice long enough and > compared to qemuDomainRemoveDiskDevice, but couldn't quite make up my > mind and really didn't want to be creating "extra" patches for perhaps > all the erroneous removals. > > If my comparison is {Attach|Remove}Disk, which has... > > Attach: Add secobj, Add encobj, Add Drive, and Add Device where drive > can rely on secobj and/or encobj. The encobj and secobj do not rely on > each other. The secobj is the possible secret for the iSCSI/RBD server, > while encobj is the possible secret for > > On attach error the removal is Del Drive, Del secobj, Del encobj (order > of secobj/encobj doesn't matter). > > Detach: Del Device and call RemoveDisk which Del secobj, Del encobj, and > Del Drive. > > Then, the RemoveDisk is probably wrong (Del Drive should be first), but > I didn't want to continue to bog down the chardev changes if my thoughts > in patch 2 were wrong. I think that the RemoveDisk is wrong and that the Del Drive should be first. As you've wrote, the drive can rely on secobj and/or encobj so we should not remove the object while they can be still used.

Pavel > > Long way of saying, I want to get it right for this path and then if I'm > right generate a RemoveDisk patch to swap order. > > John > >>> >>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >>> index e287aaf..f401b48 100644 >>> --- a/src/qemu/qemu_hotplug.c >>> +++ b/src/qemu/qemu_hotplug.c >>> @@ -3608,6 +3608,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, >>> virObjectEventPtr event; >>> char *charAlias = NULL; >>> char *objAlias = NULL; >>> + char *tlsAlias = NULL; >>> qemuDomainObjPrivatePtr priv = vm->privateData; >>> ssize_t idx; >>> int ret = -1; >>> @@ -3616,15 +3617,23 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, >>> VIR_DEBUG("Removing RNG device %s from domain %p %s", >>> rng->info.alias, vm, vm->def->name); >>> >>> + >>> if (virAsprintf(&objAlias, "obj%s", rng->info.alias) < 0) >>> goto cleanup; >>> >>> if (!(charAlias = qemuAliasChardevFromDevAlias(rng->info.alias))) >>> goto cleanup; >>> >>> + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD && >>> + !(tlsAlias = qemuAliasTLSObjFromChardevAlias(charAlias))) >>> + goto cleanup; >>> + >>> qemuDomainObjEnterMonitor(driver, vm); >>> - if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) >>> + if (rng->backend == VIR_DOMAIN_RNG_BACKEND_EGD) { >>> + if (tlsAlias) >>> + ignore_value(qemuMonitorDelObject(priv->mon, tlsAlias)); >>> ignore_value(qemuMonitorDetachCharDev(priv->mon, charAlias)); >>> + } >>> >>> rc = qemuMonitorDelObject(priv->mon, objAlias); >>> >>> @@ -3648,6 +3657,7 @@ qemuDomainRemoveRNGDevice(virQEMUDriverPtr driver, >>> cleanup: >>> VIR_FREE(charAlias); >>> VIR_FREE(objAlias); >>> + VIR_FREE(tlsAlias); >>> return ret; >>> } >>> >>> -- >>> 2.7.4 >>> >>> -- >>> libvir-list mailing list >>> libvir-list(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/libvir-list > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list