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

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