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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

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