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

[Pavel Hrdina]

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 at redhat.com>
> >> ---
> >>  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 at redhat.com
> >> https://www.redhat.com/mailman/listinfo/libvir-list
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161025/bae227b2/attachment-0001.sig>