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

Ján Tomko <jtomko@xxxxxxxxxx> · Thu, 14 Jul 2016 16:31:59 +0200

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@xxxxxxxxxx>
---
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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list






