[libvirt] [PATCH v3 08/10] qemu: Add secinfo for hotplug virtio disk

[Ján Tomko]

On Fri, Jun 24, 2016 at 04:53:37PM -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 | 42 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 41 insertions(+), 1 deletion(-)
>

ACK, terms and conditions apply.

>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index f695903..fbe3cb8 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -310,6 +310,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     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;

This seems to be the only usage of diskPriv.
You can do QEMU_DOMAIN_DISK_PRIVATE(disk)->secinfo directly.

>+    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,15 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks+1) < 0)
>         goto error;
>
>-    /* Attach the device - 2 step process */
>+    /* Attach the device - possible 3 step process */
>     qemuDomainObjEnterMonitor(driver, vm);
>
>+    if (secobjProps && qemuMonitorAddObject(priv->mon, "secret",
>+                                            secinfo->s.aes.alias,
>+                                            secobjProps) < 0)

It would be nice to set secobjProps to NULL here right after it's
invalid.

>+        goto failaddobjsecret;

s/failaddobjsecret/exit_monitor/

>+    secobjProps = NULL;
>+
>     if (qemuMonitorAddDrive(priv->mon, drivestr) < 0)
>         goto failadddrive;
>
>@@ -374,6 +390,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     ret = 0;
>
>  cleanup:
>+    virJSONValueFree(secobjProps);
>     qemuDomainSecretDiskDestroy(disk);
>     VIR_FREE(devstr);
>     VIR_FREE(drivestr);
>@@ -393,8 +410,13 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn,
>     }
>
>  failadddrive:
>+    if (secobjProps)
>+        ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias));

This can be done if (secinfo) then. Also, the error should be saved
since qemuMonitorDelObject can overwrite it.

>+
>+ failaddobjsecret:
>     if (qemuDomainObjExitMonitor(driver, vm) < 0)
>         releaseaddr = false;
>+    secobjProps = NULL; /* qemuMonitorAddObject consumes props on failure too */
>
>  failexitmonitor:
>     virDomainAuditDisk(vm, NULL, disk->src, "attach", false);
>@@ -2791,6 +2813,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);
>@@ -2801,7 +2824,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)) {
>+

Do we still have QEMU_DOMAIN_DISK_PRIVATE to check secinfo, just like we
did when adding the secret?

Jan

>+        if (!(objAlias = qemuDomainGetSecretAESAlias(disk->info.alias))) {
>+            VIR_FREE(drivestr);
>+            return -1;
>+        }
>+    }
>+
>     qemuDomainObjEnterMonitor(driver, vm);
>+
>+    /* If it fails, then so be it - it was a best shot */
>+    if (objAlias)
>+        ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
>+    VIR_FREE(objAlias);
>+
>     qemuMonitorDriveDel(priv->mon, drivestr);
>     VIR_FREE(drivestr);
>     if (qemuDomainObjExitMonitor(driver, vm) < 0)
>-- 
>2.5.5
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list