On 06/25/2016 02:43 AM, Ján Tomko wrote:
> 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.
>
Understood, but I'm not a fan of that look. I'll keep it the way it is.
>> + 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/
>
So you think it looks better to:
if (secobjProps && qemuMonitorAddObject(priv->mon, "secret",
secinfo->s.aes.alias,
secobjProps) < 0) {
secobjProps = NULL;
goto some-label-to-ExitMonitor;
}
secobjProps = NULL;
That seems to go against other exit/error paths. Is it wrong the way it
is? If not, I see no reason to change it.
>> + 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.
>
Just secinfo isn't enough, it needs to be secinfo->type ==
VIR_DOMAIN_SECRET_INFO_TYPE_AES as well... If we're _TYPE_PLAIN, then
there's nothing to do. I think keeping secobjProps is cleaner.
I added code to save the error - although it seems that every one of
those labels could end up in the same situation - going through code
that could overwrite the error. It'll be fixed for the failure labels
as a result of failure while in the monitor, but I think a future
cleanup could/should encompass all those labels.
>> +
>> + 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?
>
>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.
Tks -
John
> 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