[libvirt] [PATCH v4 5/7] qemu: Add secinfo for hotplug virtio disk

[John Ferlan]

[...]

>>     qemuDomainObjEnterMonitor(driver, vm);
>>
>> +    if (secobjProps && qemuMonitorAddObject(priv->mon, "secret",
>> +                                            secinfo->s.aes.alias,
>> +                                            secobjProps) < 0)
>> +        goto exit_monitor;
>> +
>>     if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>>         goto failadddrive;
>>
>> @@ -368,12 +382,18 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr
>> conn,
>>         goto failexitmonitor;
>>     }
>>
>> +    /* qemuMonitorAddObject consumes the props, but we need to wait
>> +     * for successful exit from monitor to clear; otherwise, error
>> +     * paths wouldn't clean up properly */
>> +    secobjProps = NULL;
>> +
> 
> I would rather set it to NULL right after the AddObject call and track
> whether we need to remove the object in a separate variable.
> 
> This way when qemuDomainObjExitMonitor fails, we jump to 'failexitmonitor'
> without setting it to NULL and free it again.
> 

OK - this is fixed up in my local branch - awaiting ACK's for the
cleanup series..

>>     virDomainAuditDisk(vm, NULL, disk->src, "attach", true);
>>
>>     virDomainDiskInsertPreAlloced(vm->def, disk);
>>     ret = 0;
>>
>>  cleanup:
>> +    virJSONValueFree(secobjProps);
>>     qemuDomainSecretDiskDestroy(disk);
>>     VIR_FREE(devstr);
>>     VIR_FREE(drivestr);
>> @@ -387,14 +407,23 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr
>> conn,
>>         VIR_WARN("Unable to remove drive %s (%s) after failed "
>>                  "qemuMonitorAddDevice", drivealias, drivestr);
>>     }
>> +
>> + failadddrive:
>> +    if (secobjProps) {
> 
>> +        if (!orig_err)
>> +            orig_err = virSaveLastError();
> 
> This will need to be repeated on every label.
> In hindsight, using a set of bools for rollback (like
> qemuDomainAttachHostUSBDevice does) might have been more readable than
> separate labels.

Well hindsight is 20/20 - so I posted another series dealing with the
error path adjustments. Hopefully that works better and then this just
is a simple addition... Already done in a local branch.  The secobjProps
processing would follow the similar mechanism as AttachRNG and
AttachMemory w/r/t "rv = qemuMonitorAddObject()", clear props, if (rv <
0), goto monitor cleanup, and setting the cleanup boolean.

> 
>> +        ignore_value(qemuMonitorDelObject(priv->mon,
>> secinfo->s.aes.alias));
>> +    }
>> +
>>     if (orig_err) {
>>         virSetError(orig_err);
>>         virFreeError(orig_err);
>>     }
>>
>> - failadddrive:
>> + exit_monitor:
>>     if (qemuDomainObjExitMonitor(driver, vm) < 0)
>>         releaseaddr = false;
>> +    secobjProps = NULL; /* qemuMonitorAddObject consumes props on
>> failure too */
>>
>>  failexitmonitor:
>>     virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
>> @@ -2808,6 +2837,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
>>     const char *src = virDomainDiskGetSource(disk);
>>     qemuDomainObjPrivatePtr priv = vm->privateData;
>>     char *drivestr;
>> +    char *objAlias = NULL;
>>
>>     VIR_DEBUG("Removing disk %s from domain %p %s",
>>               disk->info.alias, vm, vm->def->name);
>> @@ -2818,7 +2848,24 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr
>> driver,
>>                     QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0)
>>         return -1;
>>
>> +    /* Let's look for some markers for a secret object and create an
>> alias
>> +     * object to be used to attempt to delete the object that was
>> created */
>> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
>> +        qemuDomainSecretDiskCapable(disk->src)) {
>> +
>> +        if (!(objAlias =
>> qemuDomainGetSecretAESAlias(disk->info.alias))) {
> 
> Can't we access the disk private info here, to use the same conditions
> as we did for adding it?

Cut-n-paste from my response to the v3 review series
http://www.redhat.com/archives/libvir-list/2016-June/msg01999.html:

"From Peter's review comments of the previous series - no. In fact, the
cleanup of qemuProcessLaunch was designed to remove the Secrets that we
saved away temporarily so that they wouldn't be kept in memory forever
(e.g call to qemuDomainSecretDiskDestroy). Hence why the Attach
processing needs the qemuDomainSecretDiskPrepare (and also calls
qemuDomainSecretDiskDestroy).

Thus, the detach processing, then can only work off the alias to remove."

But I could augment the comment a bit.  I think libvirtd restart
wouldn't have them either...

John