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

[Ján Tomko]

On Mon, Jul 11, 2016 at 02:07:56PM -0400, John Ferlan wrote:
>Commit id 'a1344f70a' added AES secret processing for RBD when starting
>up a guest. As such, when the hotplug code calls qemuDomainSecretDiskPrepare
>an AES secret could be added to the disk about to be hotplugged. If an AES
>secret was added, then the hotplug code would need to generate the secret
>object because qemuBuildDriveStr would add the "password-secret=" to the
>returned 'driveStr' rather than the base64 encoded password.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
> src/qemu/qemu_hotplug.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 3 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index 093aaf9..004d6e3 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -303,13 +303,16 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
> {
>     int ret = -1;
>     qemuDomainObjPrivatePtr priv = vm->privateData;
>-    virErrorPtr orig_err;
>+    virErrorPtr orig_err = NULL;
>     char *devstr = NULL;
>     char *drivestr = NULL;
>     char *drivealias = NULL;
>     bool releaseaddr = false;
>     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>     const char *src = virDomainDiskGetSource(disk);
>+    virJSONValuePtr secobjProps = NULL;
>+    qemuDomainDiskPrivatePtr diskPriv;
>+    qemuDomainSecretInfoPtr secinfo;
>
>     if (!disk->info.type) {
>         if (qemuDomainMachineIsS390CCW(vm->def) &&
>@@ -342,6 +345,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0)
>         goto error;
>
>+    diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>+    secinfo = diskPriv->secinfo;
>+    if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
>+        if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0)
>+            goto error;
>+    }
>+
>     if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps)))
>         goto error;
>
>@@ -354,9 +364,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
>         goto error;
>
>-    /* Attach the device - 2 step process */
>     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.

>     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.

>+        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?

ACK with the double free fixed.

Jan