[libvirt] [PATCH 0/7] qemu: block job/disk related cleanups
I've accumulated quite a few patches in my branch to add support for allowing to use 'detect zeroes' for the block copy job. These are pathches that make sense to be posted separately. Peter Krempa (7): qemu: hotplug: Assume support for -device in qemuDomainAttachSCSIDisk qemu: monitor: Drop qemuMonitorAttachDrive and leaves in call tree qemu: monitor: Remove JSON impls of drive_add and drive_del qemu: Kill qemuDiskPathToAlias qemu: Split image access revoking from qemuDomainPrepareDiskChainElement qemu: Refactor qemuDomainPrepareDiskChainElement qemu: domain: Move and export qemuDomainDiskChainElement(Prepare|Revoke) src/qemu/qemu_alias.c | 16 ++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_domain.c | 81 +++++++++++++++++++ src/qemu/qemu_domain.h | 11 +++ src/qemu/qemu_driver.c | 187 ++++++++++++------------------------------- src/qemu/qemu_hotplug.c | 63 +++------------ src/qemu/qemu_monitor.c | 35 +++----- src/qemu/qemu_monitor.h | 6 -- src/qemu/qemu_monitor_json.c | 29 ------- src/qemu/qemu_monitor_json.h | 6 -- src/qemu/qemu_monitor_text.c | 102 +---------------------- src/qemu/qemu_monitor_text.h | 5 -- 12 files changed, 187 insertions(+), 356 deletions(-) -- 2.7.3
We've started to assume support for QEMU_CAPS_DEVICE. Doing so in the SCSI disk hotplug code allows us to drop a lot of ugly legacy code. --- src/qemu/qemu_hotplug.c | 62 ++++++++++--------------------------------------- 1 file changed, 12 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index b580283..ac359da 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -564,7 +564,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, { size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainControllerDefPtr cont = NULL; char *drivestr = NULL; char *devstr = NULL; int ret = -1; @@ -581,27 +580,11 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; } - /* Let's make sure our disk has a controller defined and loaded - * before trying add the disk. The controller the disk is going - * to use must exist before a qemu command line string is generated. - */ - for (i = 0; i <= disk->info.addr.drive.controller; i++) { - cont = qemuDomainFindOrCreateSCSIDiskController(driver, vm, i); - if (!cont) - goto error; - } - - /* Tell clang that "cont" is non-NULL. - This is because disk->info.addr.driver.controller is unsigned, - and hence the above loop must iterate at least once. */ - sa_assert(cont); + if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) + goto error; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) - goto error; - if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) - goto error; - } + if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) + goto error; if (!(drivestr = qemuBuildDriveStr(conn, disk, false, priv->qemuCaps))) goto error; @@ -610,38 +593,17 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; qemuDomainObjEnterMonitor(driver, vm); - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) { - ret = qemuMonitorAddDrive(priv->mon, drivestr); - if (ret == 0) { - ret = qemuMonitorAddDevice(priv->mon, devstr); - if (ret < 0) { - VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", - drivestr, devstr); - /* XXX should call 'drive_del' on error but this does not - exist yet */ - } - } - } else { - if (cont->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("SCSI controller %d was missing its PCI address"), - cont->idx); - goto error; - } - virDomainDeviceDriveAddress driveAddr; - ret = qemuMonitorAttachDrive(priv->mon, - drivestr, - &cont->info.addr.pci, - &driveAddr); - if (ret == 0) { - /* XXX we should probably validate that the addr matches - * our existing defined addr instead of overwriting */ - disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; - disk->info.addr.drive.bus = driveAddr.bus; - disk->info.addr.drive.unit = driveAddr.unit; + ret = qemuMonitorAddDrive(priv->mon, drivestr); + if (ret == 0) { + ret = qemuMonitorAddDevice(priv->mon, devstr); + if (ret < 0) { + VIR_WARN("qemuMonitorAddDevice failed on %s (%s)", + drivestr, devstr); + /* XXX should call 'drive_del' on error but this does not exist yet */ } } + if (qemuDomainObjExitMonitor(driver, vm) < 0) { ret = -1; goto error; -- 2.7.3
After the refactor in the previous patch, the functions are no longer required. --- src/qemu/qemu_hotplug.c | 1 - src/qemu/qemu_monitor.c | 16 -------- src/qemu/qemu_monitor.h | 6 --- src/qemu/qemu_monitor_text.c | 98 -------------------------------------------- src/qemu/qemu_monitor_text.h | 5 --- 5 files changed, 126 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ac359da..bc2336a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -562,7 +562,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - size_t i; qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr = NULL; char *devstr = NULL; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ace3bb4..3d4443d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2782,22 +2782,6 @@ qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, int -qemuMonitorAttachDrive(qemuMonitorPtr mon, - const char *drivestr, - virDevicePCIAddress *controllerAddr, - virDomainDeviceDriveAddress *driveAddr) -{ - VIR_DEBUG("drivestr=%s domain=%d bus=%d slot=%d function=%d", - drivestr, controllerAddr->domain, controllerAddr->bus, - controllerAddr->slot, controllerAddr->function); - - QEMU_CHECK_MONITOR_JSON(mon); - - return qemuMonitorTextAttachDrive(mon, drivestr, controllerAddr, driveAddr); -} - - -int qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, qemuMonitorPCIAddress **addrs) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4467a41..b752a98 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -672,12 +672,6 @@ int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virDevicePCIAddress *guestAddr); -int qemuMonitorAttachDrive(qemuMonitorPtr mon, - const char *drivestr, - virDevicePCIAddress *controllerAddr, - virDomainDeviceDriveAddress *driveAddr); - - typedef struct _qemuMonitorPCIAddress qemuMonitorPCIAddress; struct _qemuMonitorPCIAddress { unsigned int vendor; diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index bb87397..9703e36 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2213,104 +2213,6 @@ int qemuMonitorTextAttachPCIDiskController(qemuMonitorPtr mon, } -static int -qemuParseDriveAddReply(const char *reply, - virDomainDeviceDriveAddressPtr addr) -{ - char *s, *e; - - /* If the command succeeds qemu prints: - * OK bus X, unit Y - */ - - if (!(s = strstr(reply, "OK "))) - return -1; - - s += 3; - - if (STRPREFIX(s, "bus ")) { - s += strlen("bus "); - - if (virStrToLong_ui(s, &e, 10, &addr->bus) == -1) { - VIR_WARN("Unable to parse bus '%s'", s); - return -1; - } - - if (!STRPREFIX(e, ", ")) { - VIR_WARN("Expected ', ' parsing drive_add reply '%s'", s); - return -1; - } - s = e + 2; - } - - if (!STRPREFIX(s, "unit ")) { - VIR_WARN("Expected 'unit ' parsing drive_add reply '%s'", s); - return -1; - } - s += strlen("bus "); - - if (virStrToLong_ui(s, &e, 10, &addr->unit) == -1) { - VIR_WARN("Unable to parse unit number '%s'", s); - return -1; - } - - return 0; -} - - -int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, - const char *drivestr, - virDevicePCIAddress *controllerAddr, - virDomainDeviceDriveAddress *driveAddr) -{ - char *cmd = NULL; - char *reply = NULL; - int ret = -1; - char *safe_str; - bool tryOldSyntax = false; - - safe_str = qemuMonitorEscapeArg(drivestr); - if (!safe_str) - return -1; - - try_command: - if (virAsprintf(&cmd, "drive_add %s%.2x:%.2x:%.2x %s", - (tryOldSyntax ? "" : "pci_addr="), - controllerAddr->domain, controllerAddr->bus, - controllerAddr->slot, safe_str) < 0) - goto cleanup; - - if (qemuMonitorHMPCommand(mon, cmd, &reply) < 0) - goto cleanup; - - if (strstr(reply, "unknown command:")) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("drive hotplug is not supported")); - goto cleanup; - } - - if (qemuParseDriveAddReply(reply, driveAddr) < 0) { - if (!tryOldSyntax && strstr(reply, "invalid char in expression")) { - VIR_FREE(reply); - VIR_FREE(cmd); - tryOldSyntax = true; - goto try_command; - } - virReportError(VIR_ERR_OPERATION_FAILED, - _("adding %s disk failed: %s"), drivestr, reply); - goto cleanup; - } - - ret = 0; - - cleanup: - VIR_FREE(cmd); - VIR_FREE(reply); - VIR_FREE(safe_str); - return ret; -} - - /* * The format we're after looks like this * diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 44a5330..287a851 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -173,11 +173,6 @@ int qemuMonitorTextAttachPCIDiskController(qemuMonitorPtr mon, const char *bus, virDevicePCIAddress *guestAddr); -int qemuMonitorTextAttachDrive(qemuMonitorPtr mon, - const char *drivestr, - virDevicePCIAddress *controllerAddr, - virDomainDeviceDriveAddress *driveAddr); - int qemuMonitorTextGetAllPCIAddresses(qemuMonitorPtr mon, qemuMonitorPCIAddress **addrs); -- 2.7.3
On 03/18/2016 09:26 AM, Peter Krempa wrote:
After the refactor in the previous patch, the functions are no longer required.
Functions no longer required for attaching SCSI disks since QEMU_CAPS_DEVICE is expected.
--- src/qemu/qemu_hotplug.c | 1 - src/qemu/qemu_monitor.c | 16 -------- src/qemu/qemu_monitor.h | 6 --- src/qemu/qemu_monitor_text.c | 98 -------------------------------------------- src/qemu/qemu_monitor_text.h | 5 --- 5 files changed, 126 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ac359da..bc2336a 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -562,7 +562,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virDomainObjPtr vm, virDomainDiskDefPtr disk) { - size_t i;
^^ Belongs in previous patch; otherwise, git bisect is broken
qemuDomainObjPrivatePtr priv = vm->privateData; char *drivestr = NULL; char *devstr = NULL;
[...] ACK w/ change (yes, that implies patch 1 ACK) John
qemu won't ever add those functions directly to QMP. They will be replaced with 'blockdev-add' and 'blockdev-del' eventually. At this time there's no need to keep the stubs around. Additionally the drive_del stub in JSON contained dead code in the attempt to report errors. (VIR_ERR_OPERATION_UNSUPPORTED was never reported). Since the text impl does have the same message it is reported anyways. --- src/qemu/qemu_monitor.c | 19 +++++++++++-------- src/qemu/qemu_monitor_json.c | 29 ----------------------------- src/qemu/qemu_monitor_json.h | 6 ------ src/qemu/qemu_monitor_text.c | 4 +--- 4 files changed, 12 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 3d4443d..56df969 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2796,6 +2796,13 @@ qemuMonitorGetAllPCIAddresses(qemuMonitorPtr mon, } +/** + * qemuMonitorDriveDel: + * @mon: monitor object + * @drivestr: identifier of drive to delete. + * + * Attempts to remove a host drive. + * Returns 1 if unsupported, 0 if ok, and -1 on other failure */ int qemuMonitorDriveDel(qemuMonitorPtr mon, const char *drivestr) @@ -2804,10 +2811,8 @@ qemuMonitorDriveDel(qemuMonitorPtr mon, QEMU_CHECK_MONITOR(mon); - if (mon->json) - return qemuMonitorJSONDriveDel(mon, drivestr); - else - return qemuMonitorTextDriveDel(mon, drivestr); + /* there won't be a direct replacement for drive_del in QMP */ + return qemuMonitorTextDriveDel(mon, drivestr); } @@ -2910,10 +2915,8 @@ qemuMonitorAddDrive(qemuMonitorPtr mon, QEMU_CHECK_MONITOR(mon); - if (mon->json) - return qemuMonitorJSONAddDrive(mon, drivestr); - else - return qemuMonitorTextAddDrive(mon, drivestr); + /* there won't ever be a direct QMP replacement for this function */ + return qemuMonitorTextAddDrive(mon, drivestr); } diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 8352e53..fad948f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3592,35 +3592,6 @@ int qemuMonitorJSONDelObject(qemuMonitorPtr mon, } -int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, - const char *drivestr) -{ - /* XXX Update to use QMP, if QMP ever adds support for drive_add */ - VIR_DEBUG("drive_add command not found, trying HMP"); - return qemuMonitorTextAddDrive(mon, drivestr); -} - - -int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, - const char *drivestr) -{ - int ret; - - /* XXX Update to use QMP, if QMP ever adds support for drive_del */ - VIR_DEBUG("drive_del command not found, trying HMP"); - if ((ret = qemuMonitorTextDriveDel(mon, drivestr)) < 0) { - virErrorPtr err = virGetLastError(); - if (err && err->code == VIR_ERR_OPERATION_UNSUPPORTED) { - VIR_ERROR("%s", - _("deleting disk is not supported. " - "This may leak data if disk is reassigned")); - ret = 1; - virResetLastError(); - } - } - return ret; -} - int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, const char *alias, const char *passphrase) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 4068187..0b4916b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -230,12 +230,6 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon, int qemuMonitorJSONDelObject(qemuMonitorPtr mon, const char *objalias); -int qemuMonitorJSONAddDrive(qemuMonitorPtr mon, - const char *drivestr); - -int qemuMonitorJSONDriveDel(qemuMonitorPtr mon, - const char *drivestr); - int qemuMonitorJSONSetDrivePassphrase(qemuMonitorPtr mon, const char *alias, const char *passphrase); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 9703e36..7ec50e2 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -2457,8 +2457,7 @@ int qemuMonitorTextAddDrive(qemuMonitorPtr mon, return ret; } -/* Attempts to remove a host drive. - * Returns 1 if unsupported, 0 if ok, and -1 on other failure */ + int qemuMonitorTextDriveDel(qemuMonitorPtr mon, const char *drivestr) { @@ -2466,7 +2465,6 @@ int qemuMonitorTextDriveDel(qemuMonitorPtr mon, char *reply = NULL; char *safedev; int ret = -1; - VIR_DEBUG("TextDriveDel drivestr=%s", drivestr); if (!(safedev = qemuMonitorEscapeArg(drivestr))) goto cleanup; -- 2.7.3
The function has terrible semantics. Split it into two functions. --- src/qemu/qemu_alias.c | 16 ++++++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_domain.c | 16 ++++++++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 99 +++++++++++++++++++++----------------------------- 5 files changed, 79 insertions(+), 57 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index efd9222..9691223 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -484,3 +484,19 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return 0; } + + +char * +qemuAliasFromDisk(const virDomainDiskDef *disk) +{ + char *ret; + + if (!disk->info.alias) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("disk does not have an alias")); + return NULL; + } + + ignore_value(virAsprintf(&ret, "drive-%s", disk->info.alias)); + + return ret; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index a2eaa27..714a526 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -61,4 +61,6 @@ int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, const char *prefix); + +char *qemuAliasFromDisk(const virDomainDiskDef *disk); #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 594063e..0043e9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4547,3 +4547,19 @@ qemuDomainNetVLAN(virDomainNetDefPtr def) { return qemuDomainDeviceAliasIndex(&def->info, "net"); } + + +virDomainDiskDefPtr +qemuDomainDiskByName(virDomainDefPtr def, + const char *name) +{ + virDomainDiskDefPtr ret; + + if (!(ret = virDomainDiskByName(def, name, true))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("No device found for specified path")); + return NULL; + } + + return ret; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 5976455..8292ca9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -545,4 +545,7 @@ int qemuDomainSetPrivatePaths(char **domainLibDir, const char *confChannelDir, const char *domainName, int domainId); + +virDomainDiskDefPtr qemuDomainDiskByName(virDomainDefPtr def, const char *name); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 08e784b..6a008b4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16031,33 +16031,6 @@ qemuDomainOpenChannel(virDomainPtr dom, return ret; } -static char * -qemuDiskPathToAlias(virDomainObjPtr vm, const char *path, int *idxret) -{ - int idx; - char *ret = NULL; - virDomainDiskDefPtr disk; - - idx = virDomainDiskIndexByName(vm->def, path, true); - if (idx < 0) - goto cleanup; - - disk = vm->def->disks[idx]; - if (idxret) - *idxret = idx; - - if (virDomainDiskGetSource(disk)) { - if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) - return NULL; - } - - cleanup: - if (!ret) { - virReportError(VIR_ERR_INVALID_ARG, - "%s", _("No device found for specified path")); - } - return ret; -} /* Called while holding the VM job lock, to implement a block job * abort with pivot; this updates the VM definition as appropriate, on @@ -16179,7 +16152,6 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; bool modern; - int idx; virDomainDiskDefPtr disk; virStorageSourcePtr baseSource = NULL; unsigned int baseIndex = 0; @@ -16223,9 +16195,11 @@ qemuDomainBlockPullCommon(virQEMUDriverPtr driver, } } - if (!(device = qemuDiskPathToAlias(vm, path, &idx))) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; - disk = vm->def->disks[idx]; if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; @@ -16306,7 +16280,6 @@ qemuDomainBlockJobAbort(virDomainPtr dom, virDomainDiskDefPtr disk = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); bool save = false; - int idx; bool modern; bool pivot = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT); bool async = !!(flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC); @@ -16334,9 +16307,11 @@ qemuDomainBlockJobAbort(virDomainPtr dom, if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto endjob; - if (!(device = qemuDiskPathToAlias(vm, path, &idx))) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; - disk = vm->def->disks[idx]; if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_NONE && disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { @@ -16517,6 +16492,7 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; + virDomainDiskDefPtr disk; int ret = -1; virDomainObjPtr vm; bool modern; @@ -16554,7 +16530,10 @@ qemuDomainBlockJobSetSpeed(virDomainPtr dom, if (qemuDomainSupportsBlockJobs(vm, &modern) < 0) goto endjob; - if (!(device = qemuDiskPathToAlias(vm, path, NULL))) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; qemuDomainObjEnterMonitor(driver, vm); @@ -16593,7 +16572,6 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, char *device = NULL; virDomainDiskDefPtr disk = NULL; int ret = -1; - int idx; struct stat st; bool need_unlink = false; virQEMUDriverConfigPtr cfg = NULL; @@ -16616,10 +16594,12 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, goto endjob; } - device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; - disk = vm->def->disks[idx]; + if (qemuDomainDiskBlockJobIsActive(disk)) goto endjob; @@ -16950,7 +16930,6 @@ qemuDomainBlockCommit(virDomainPtr dom, virDomainObjPtr vm = NULL; char *device = NULL; int ret = -1; - int idx; virDomainDiskDefPtr disk = NULL; virStorageSourcePtr topSource; unsigned int topIndex = 0; @@ -17005,10 +16984,11 @@ qemuDomainBlockCommit(virDomainPtr dom, speed <<= 20; } - device = qemuDiskPathToAlias(vm, path, &idx); - if (!device) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; - disk = vm->def->disks[idx]; if (!disk->src->path) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -17325,7 +17305,7 @@ qemuDomainOpenGraphicsFD(virDomainPtr dom, static int qemuDomainSetBlockIoTune(virDomainPtr dom, - const char *disk, + const char *path, virTypedParameterPtr params, int nparams, unsigned int flags) @@ -17339,8 +17319,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, char *device = NULL; int ret = -1; size_t i; - int idx = -1; virDomainDiskDefPtr conf_disk = NULL; + virDomainDiskDefPtr disk; bool set_bytes = false; bool set_iops = false; bool set_bytes_max = false; @@ -17409,7 +17389,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, disk) < 0) + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) goto endjob; for (i = 0; i < nparams; i++) { @@ -17574,10 +17554,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!(conf_disk = virDomainDiskByName(persistentDef, disk, true))) { + if (!(conf_disk = virDomainDiskByName(persistentDef, path, true))) { virReportError(VIR_ERR_INVALID_ARG, _("missing persistent configuration for disk '%s'"), - disk); + path); goto endjob; } } @@ -17600,13 +17580,16 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (!(device = qemuDiskPathToAlias(vm, disk, &idx))) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; /* If the user didn't specify bytes limits, inherit previous * values; likewise if the user didn't specify iops * limits. */ - oldinfo = &vm->def->disks[idx]->blkdeviotune; + oldinfo = &disk->blkdeviotune; if (!set_bytes) { info.total_bytes_sec = oldinfo->total_bytes_sec; info.read_bytes_sec = oldinfo->read_bytes_sec; @@ -17636,7 +17619,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, ret = -1; if (ret < 0) goto endjob; - vm->def->disks[idx]->blkdeviotune = info; + disk->blkdeviotune = info; ret = virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps); if (ret < 0) @@ -17683,11 +17666,12 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, static int qemuDomainGetBlockIoTune(virDomainPtr dom, - const char *disk, + const char *path, virTypedParameterPtr params, int *nparams, unsigned int flags) { + virDomainDiskDefPtr disk; virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm = NULL; qemuDomainObjPrivatePtr priv = NULL; @@ -17747,8 +17731,10 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - device = qemuDiskPathToAlias(vm, disk, NULL); - if (!device) + if (!(disk = qemuDomainDiskByName(vm->def, path))) + goto endjob; + + if (!(device = qemuAliasFromDisk(disk))) goto endjob; qemuDomainObjEnterMonitor(driver, vm); ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply, supportMaxOptions); @@ -17759,14 +17745,13 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - virDomainDiskDefPtr diskDef; - if (!(diskDef = virDomainDiskByName(persistentDef, disk, true))) { + if (!(disk = virDomainDiskByName(persistentDef, path, true))) { virReportError(VIR_ERR_INVALID_ARG, _("disk '%s' was not found in the domain config"), - disk); + path); goto endjob; } - reply = diskDef->blkdeviotune; + reply = disk->blkdeviotune; } for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM_MAX && i < *nparams; i++) { -- 2.7.3
Introduce qemuDomainDiskChainElementRevoke that revokes the access rather than having a flag to do so. --- src/qemu/qemu_driver.c | 71 +++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6a008b4..3af63aa 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13371,16 +13371,39 @@ qemuDomainMigrateGetMaxSpeed(virDomainPtr dom, typedef enum { - VIR_DISK_CHAIN_NO_ACCESS, VIR_DISK_CHAIN_READ_ONLY, VIR_DISK_CHAIN_READ_WRITE, } qemuDomainDiskChainMode; -/* Several operations end up adding or removing a single element of a disk + +/** + * qemuDomainDiskChainElementRevoke: + * + * Revoke access to a single backing chain element. This restores the labels, + * removes cgroup ACLs for devices and removes locks. + */ +static void +qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem) +{ + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, elem) < 0) + VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); + + if (qemuTeardownImageCgroup(vm, elem) < 0) + VIR_WARN("Failed to teardown cgroup for disk path %s", + NULLSTR(elem->path)); + + if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) + VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path)); +} + + +/* Several operations end up adding a single element of a disk * backing file chain; this helper function ensures that the lock manager, * cgroup device controller, and security manager labelling are all aware of - * each new file before it is added to a chain, and can revoke access to a file - * no longer needed in a chain. */ + * each new file before it is added to a chain */ static int qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -13395,28 +13418,15 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, elem->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; - if (mode == VIR_DISK_CHAIN_NO_ACCESS) { - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, elem) < 0) - VIR_WARN("Unable to restore security label on %s", elem->path); - - if (qemuTeardownImageCgroup(vm, elem) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", elem->path); - - if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) - VIR_WARN("Unable to release lock on %s", elem->path); - } else { - if (virDomainLockImageAttach(driver->lockManager, cfg->uri, - vm, elem) < 0) - goto cleanup; + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) + goto cleanup; - if (qemuSetupImageCgroup(vm, elem) < 0) - goto cleanup; + if (qemuSetupImageCgroup(vm, elem) < 0) + goto cleanup; - if (virSecurityManagerSetImageLabel(driver->securityManager, - vm->def, elem) < 0) - goto cleanup; - } + if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, + elem) < 0) + goto cleanup; ret = 0; @@ -14148,8 +14158,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* set correct security, cgroup and locking options on the new image */ if (qemuDomainPrepareDiskChainElement(driver, vm, newDiskSrc, VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, newDiskSrc, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainDiskChainElementRevoke(driver, vm, newDiskSrc); goto cleanup; } @@ -14215,8 +14224,8 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, ignore_value(virStorageFileInit(disk->src)); - qemuDomainPrepareDiskChainElement(driver, vm, disk->src, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainDiskChainElementRevoke(driver, vm, disk->src); + if (need_unlink && virStorageFileStat(disk->src, &st) == 0 && S_ISREG(st.st_mode) && virStorageFileUnlink(disk->src) < 0) @@ -16706,8 +16715,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainPrepareDiskChainElement(driver, vm, mirror, VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, mirror, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainDiskChainElementRevoke(driver, vm, mirror); goto endjob; } @@ -16719,8 +16727,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, mirror, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainDiskChainElementRevoke(driver, vm, mirror); goto endjob; } -- 2.7.3
Now that there are only two elements in the enum, let's change it to a bool and rename the function similarly to the one added in previous commit. --- src/qemu/qemu_driver.c | 44 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3af63aa..92087fc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13370,12 +13370,6 @@ qemuDomainMigrateGetMaxSpeed(virDomainPtr dom, } -typedef enum { - VIR_DISK_CHAIN_READ_ONLY, - VIR_DISK_CHAIN_READ_WRITE, -} qemuDomainDiskChainMode; - - /** * qemuDomainDiskChainElementRevoke: * @@ -13400,23 +13394,25 @@ qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, } -/* Several operations end up adding a single element of a disk - * backing file chain; this helper function ensures that the lock manager, - * cgroup device controller, and security manager labelling are all aware of - * each new file before it is added to a chain */ +/** + * qemuDomainDiskChainElementPrepare: + * + * Allow a VM access to a single element of a disk backing chain; this helper + * ensures that the lock manager, cgroup device controller, and security manager + * labelling are all aware of each new file before it is added to a chain */ static int -qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, +qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr elem, - qemuDomainDiskChainMode mode) + bool readonly) { - bool readonly = elem->readonly; + bool was_readonly = elem->readonly; virQEMUDriverConfigPtr cfg = NULL; int ret = -1; cfg = virQEMUDriverGetConfig(driver); - elem->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; + elem->readonly = readonly; if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) goto cleanup; @@ -13431,7 +13427,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, ret = 0; cleanup: - elem->readonly = readonly; + elem->readonly = was_readonly; virObjectUnref(cfg); return ret; } @@ -14156,8 +14152,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* set correct security, cgroup and locking options on the new image */ - if (qemuDomainPrepareDiskChainElement(driver, vm, newDiskSrc, - VIR_DISK_CHAIN_READ_WRITE) < 0) { + if (qemuDomainDiskChainElementPrepare(driver, vm, newDiskSrc, false) < 0) { qemuDomainDiskChainElementRevoke(driver, vm, newDiskSrc); goto cleanup; } @@ -16713,8 +16708,7 @@ qemuDomainBlockCopyCommon(virDomainObjPtr vm, keepParentLabel) < 0) goto endjob; - if (qemuDomainPrepareDiskChainElement(driver, vm, mirror, - VIR_DISK_CHAIN_READ_WRITE) < 0) { + if (qemuDomainDiskChainElementPrepare(driver, vm, mirror, false) < 0) { qemuDomainDiskChainElementRevoke(driver, vm, mirror); goto endjob; } @@ -17078,11 +17072,9 @@ qemuDomainBlockCommit(virDomainPtr dom, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; - if (qemuDomainPrepareDiskChainElement(driver, vm, baseSource, - VIR_DISK_CHAIN_READ_WRITE) < 0 || + if (qemuDomainDiskChainElementPrepare(driver, vm, baseSource, false) < 0 || (top_parent && top_parent != disk->src && - qemuDomainPrepareDiskChainElement(driver, vm, top_parent, - VIR_DISK_CHAIN_READ_WRITE) < 0)) + qemuDomainDiskChainElementPrepare(driver, vm, top_parent, false) < 0)) goto endjob; if (flags & VIR_DOMAIN_BLOCK_COMMIT_RELATIVE && @@ -17150,11 +17142,9 @@ qemuDomainBlockCommit(virDomainPtr dom, endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ - qemuDomainPrepareDiskChainElement(driver, vm, baseSource, - VIR_DISK_CHAIN_READ_ONLY); + qemuDomainDiskChainElementPrepare(driver, vm, baseSource, true); if (top_parent && top_parent != disk->src) - qemuDomainPrepareDiskChainElement(driver, vm, top_parent, - VIR_DISK_CHAIN_READ_ONLY); + qemuDomainDiskChainElementPrepare(driver, vm, top_parent, true); } virStorageSourceFree(mirror); qemuDomainObjEndJob(driver, vm); -- 2.7.3
Move the function to qemu_domain.c and export them for further use. --- src/qemu/qemu_domain.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 8 +++++++ src/qemu/qemu_driver.c | 63 ------------------------------------------------ 3 files changed, 73 insertions(+), 63 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0043e9e..9d300ad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -25,6 +25,7 @@ #include "qemu_domain.h" #include "qemu_alias.h" +#include "qemu_cgroup.h" #include "qemu_command.h" #include "qemu_parse_command.h" #include "qemu_capabilities.h" @@ -45,6 +46,7 @@ #include "viratomic.h" #include "virprocess.h" #include "logging/log_manager.h" +#include "locking/domain_lock.h" #include "storage/storage_driver.h" @@ -3358,6 +3360,69 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, } +/** + * qemuDomainDiskChainElementRevoke: + * + * Revoke access to a single backing chain element. This restores the labels, + * removes cgroup ACLs for devices and removes locks. + */ +void +qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem) +{ + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, elem) < 0) + VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); + + if (qemuTeardownImageCgroup(vm, elem) < 0) + VIR_WARN("Failed to teardown cgroup for disk path %s", + NULLSTR(elem->path)); + + if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) + VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path)); +} + + +/** + * qemuDomainDiskChainElementPrepare: + * + * Allow a VM access to a single element of a disk backing chain; this helper + * ensures that the lock manager, cgroup device controller, and security manager + * labelling are all aware of each new file before it is added to a chain */ +int +qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem, + bool readonly) +{ + bool was_readonly = elem->readonly; + virQEMUDriverConfigPtr cfg = NULL; + int ret = -1; + + cfg = virQEMUDriverGetConfig(driver); + + elem->readonly = readonly; + + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) + goto cleanup; + + if (qemuSetupImageCgroup(vm, elem) < 0) + goto cleanup; + + if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, + elem) < 0) + goto cleanup; + + ret = 0; + + cleanup: + elem->readonly = was_readonly; + virObjectUnref(cfg); + return ret; +} + + bool qemuDomainDiskSourceDiffers(virConnectPtr conn, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8292ca9..174ffbd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -456,6 +456,14 @@ int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virStorageSourcePtr src); char *qemuDomainStorageAlias(const char *device, int depth); +void qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem); +int qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr elem, + bool readonly); + int qemuDomainCleanupAdd(virDomainObjPtr vm, qemuDomainCleanupCallback cb); void qemuDomainCleanupRemove(virDomainObjPtr vm, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 92087fc..dfa9b0a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13370,69 +13370,6 @@ qemuDomainMigrateGetMaxSpeed(virDomainPtr dom, } -/** - * qemuDomainDiskChainElementRevoke: - * - * Revoke access to a single backing chain element. This restores the labels, - * removes cgroup ACLs for devices and removes locks. - */ -static void -qemuDomainDiskChainElementRevoke(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem) -{ - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, elem) < 0) - VIR_WARN("Unable to restore security label on %s", NULLSTR(elem->path)); - - if (qemuTeardownImageCgroup(vm, elem) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", - NULLSTR(elem->path)); - - if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) - VIR_WARN("Unable to release lock on %s", NULLSTR(elem->path)); -} - - -/** - * qemuDomainDiskChainElementPrepare: - * - * Allow a VM access to a single element of a disk backing chain; this helper - * ensures that the lock manager, cgroup device controller, and security manager - * labelling are all aware of each new file before it is added to a chain */ -static int -qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virStorageSourcePtr elem, - bool readonly) -{ - bool was_readonly = elem->readonly; - virQEMUDriverConfigPtr cfg = NULL; - int ret = -1; - - cfg = virQEMUDriverGetConfig(driver); - - elem->readonly = readonly; - - if (virDomainLockImageAttach(driver->lockManager, cfg->uri, vm, elem) < 0) - goto cleanup; - - if (qemuSetupImageCgroup(vm, elem) < 0) - goto cleanup; - - if (virSecurityManagerSetImageLabel(driver->securityManager, vm->def, - elem) < 0) - goto cleanup; - - ret = 0; - - cleanup: - elem->readonly = was_readonly; - virObjectUnref(cfg); - return ret; -} - - /* Return -1 if request is not sent to agent due to misconfig, -2 if request * is sent but failed, and number of frozen filesystems on success. If -2 is * returned, FSThaw should be called revert the quiesced status. */ -- 2.7.3
On 03/18/2016 09:25 AM, Peter Krempa wrote:
I've accumulated quite a few patches in my branch to add support for allowing to use 'detect zeroes' for the block copy job. These are pathches that make sense to be posted separately.
Peter Krempa (7): qemu: hotplug: Assume support for -device in qemuDomainAttachSCSIDisk qemu: monitor: Drop qemuMonitorAttachDrive and leaves in call tree qemu: monitor: Remove JSON impls of drive_add and drive_del qemu: Kill qemuDiskPathToAlias qemu: Split image access revoking from qemuDomainPrepareDiskChainElement qemu: Refactor qemuDomainPrepareDiskChainElement qemu: domain: Move and export qemuDomainDiskChainElement(Prepare|Revoke)
src/qemu/qemu_alias.c | 16 ++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_domain.c | 81 +++++++++++++++++++ src/qemu/qemu_domain.h | 11 +++ src/qemu/qemu_driver.c | 187 ++++++++++++------------------------------- src/qemu/qemu_hotplug.c | 63 +++------------ src/qemu/qemu_monitor.c | 35 +++----- src/qemu/qemu_monitor.h | 6 -- src/qemu/qemu_monitor_json.c | 29 ------- src/qemu/qemu_monitor_json.h | 6 -- src/qemu/qemu_monitor_text.c | 102 +---------------------- src/qemu/qemu_monitor_text.h | 5 -- 12 files changed, 187 insertions(+), 356 deletions(-)
ACK series, with patch 1,2 adjustment John
participants (2)
-
John Ferlan -
Peter Krempa