Upstream qemu 1.3 is adding two new monitor commands, 'drive-mirror' and 'block-job-complete'[1], which can drive live block copy and storage migration. Additionally, RHEL 6.3 had backported an earlier version of most of the same functionality, but under the names '__com.redhat_drive-mirror' and '__com.redhat_drive-reopen' and with slightly different JSON arguments, which we can support without too much extra effort. The libvirt API virDomainBlockRebase as already committed for 0.9.12 is flexible enough to expose the basics of block copy, but some additional features in the 'drive-mirror' qemu command, such as setting granularity or using a persistent bitmap, may later require a new libvirt API virDomainBlockCopy. The use of two capability bits allows the bulk of libvirt code to handle both RHEL 6.3 and upstream qemu 1.3 without caring about the difference in monitor commands used; one bit that says whether the commands are present, and another saying whether the RHEL 6.3 limitations are in effect. The choice of XML names for the capability bits is essential for interoperability with the RHEL 6.3 version of libvirt. [TODO - link to Paolo's v2 post in September, once he posts it] [1]https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg03082.html * src/qemu/qemu_capabilities.h (QEMU_CAPS_DRIVE_MIRROR) (QEMU_CAPS_DRIVE_REOPEN): New bits. * src/qemu/qemu_capabilities.c (qemuCaps): Name them. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONCheckCommands): Set them. (qemuMonitorJSONDriveMirror, qemuMonitorDrivePivot): New functions. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDriveMirror) (qemuMonitorDrivePivot): Declare them. * src/qemu/qemu_monitor.c (qemuMonitorDriveMirror) (qemuMonitorDrivePivot): New passthroughs. * src/qemu/qemu_monitor.h (qemuMonitorDriveMirror) (qemuMonitorDrivePivot): Declare them. --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 2 ++ src/qemu/qemu_monitor.c | 52 ++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 14 ++++++++ src/qemu/qemu_monitor_json.c | 82 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 21 ++++++++++-- 6 files changed, 171 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 43645bf..fcf1e47 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -177,6 +177,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "disable-s4", /* 105 */ "usb-redir.filter", + "drive-mirror", + "drive-reopen", ); struct _qemuCaps { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index c1519ed..f04451f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -142,6 +142,8 @@ enum qemuCapsFlags { QEMU_CAPS_DISABLE_S3 = 104, /* S3 BIOS Advertisement on/off */ QEMU_CAPS_DISABLE_S4 = 105, /* S4 BIOS Advertisement on/off */ QEMU_CAPS_USB_REDIR_FILTER = 106, /* usb-redir.filter */ + QEMU_CAPS_DRIVE_MIRROR = 107, /* drive-mirror monitor command */ + QEMU_CAPS_DRIVE_REOPEN = 108, /* drive-reopen instead of block-job-complete */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f8d717f..b7730fd 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2749,6 +2749,39 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, return ret; } +/* Start a drive-mirror block job. bandwidth is in MB/sec. */ +int +qemuMonitorDriveMirror(qemuMonitorPtr mon, + const char *device, const char *file, + const char *format, unsigned long bandwidth, + bool reopen, unsigned int flags) +{ + int ret = -1; + unsigned long long speed; + + VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, bandwidth=%ld, " + "reopen=%d, flags=%x", + mon, device, file, NULLSTR(format), bandwidth, reopen, flags); + + /* Convert bandwidth MiB to bytes */ + speed = bandwidth; + if (speed > ULLONG_MAX / 1024 / 1024) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth must be less than %llu"), + ULLONG_MAX / 1024 / 1024); + return -1; + } + speed = bandwidth * 1024ULL * 1024ULL; + + if (mon->json) + ret = qemuMonitorJSONDriveMirror(mon, device, file, format, speed, + reopen, flags); + else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("drive-mirror requires JSON monitor")); + return ret; +} + /* Use the transaction QMP command to run atomic snapshot commands. */ int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) @@ -2765,6 +2798,25 @@ qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) return ret; } +/* Use the block-job-complete or drive-reopen monitor command to pivot + * a block copy job. */ +int +qemuMonitorDrivePivot(qemuMonitorPtr mon, const char *device, + const char *file, const char *format, bool reopen) +{ + int ret = -1; + + VIR_DEBUG("mon=%p, device=%s, file=%s, format=%s, reopen=%d", + mon, device, file, NULLSTR(format), reopen); + + if (mon->json) + ret = qemuMonitorJSONDrivePivot(mon, device, file, format, reopen); + else + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("drive pivot requires JSON monitor")); + return ret; +} + int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, char **reply, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d44e93c..e37dac8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -530,6 +530,20 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, bool reuse); int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorDriveMirror(qemuMonitorPtr mon, + const char *device, + const char *file, + const char *format, + unsigned long bandwidth, + bool reopen, + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int qemuMonitorDrivePivot(qemuMonitorPtr mon, + const char *device, + const char *file, + const char *format, + bool reopen) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ed18a64..a223c08 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1012,6 +1012,10 @@ qemuMonitorJSONCheckCommands(qemuMonitorPtr mon, qemuCapsSet(caps, QEMU_CAPS_BLOCKJOB_ASYNC); else if (STREQ(name, "dump-guest-memory")) qemuCapsSet(caps, QEMU_CAPS_DUMP_GUEST_MEMORY); + else if (strstr(name, "drive-mirror")) + qemuCapsSet(caps, QEMU_CAPS_DRIVE_MIRROR); + else if (STREQ(name, "__com.redhat_drive-reopen")) + qemuCapsSet(caps, QEMU_CAPS_DRIVE_REOPEN); } ret = 0; @@ -3304,6 +3308,52 @@ cleanup: return ret; } +/* speed is in bytes/sec */ +int +qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, + const char *device, const char *file, + const char *format, unsigned long long speed, + bool reopen, unsigned int flags) +{ + int ret = -1; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + bool shallow = (flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) != 0; + bool reuse = (flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) != 0; + + if (reopen) + cmd = qemuMonitorJSONMakeCommand("__com.redhat_drive-mirror", + "s:device", device, + "s:target", file, + "U:speed", speed, + "b:full", !shallow, + "s:mode", + reuse ? "existing" : "absolute-paths", + format ? "s:format" : NULL, format, + NULL); + else + cmd = qemuMonitorJSONMakeCommand("drive-mirror", + "s:device", device, + "s:target", file, + "U:speed", speed, + "s:sync", shallow ? "top" : "full", + "s:mode", + reuse ? "existing" : "absolute-paths", + format ? "s:format" : NULL, format, + NULL); + if (!cmd) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + ret = qemuMonitorJSONCheckError(cmd, reply); + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) { @@ -3332,6 +3382,38 @@ cleanup: return ret; } +int +qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, const char *device, + const char *file, const char *format, bool reopen) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + if (reopen) + cmd = qemuMonitorJSONMakeCommand("__com.redhat_drive-reopen", + "s:device", device, + "s:new-image-file", file, + format ? "s:format" : NULL, format, + NULL); + else + cmd = qemuMonitorJSONMakeCommand("block-job-complete", + "s:device", device, + NULL); + if (!cmd) + return -1; + + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + + ret = qemuMonitorJSONCheckError(cmd, reply); + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, char **reply_str, diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index d092b88..bdcf819 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -237,8 +237,25 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, const char *device, const char *file, const char *format, - bool reuse); -int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions); + bool reuse) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); +int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon, + const char *device, + const char *file, + const char *format, + unsigned long long speed, + bool reopen, + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +int qemuMonitorJSONDrivePivot(qemuMonitorPtr mon, + const char *device, + const char *file, + const char *format, + bool reopen) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, -- 1.7.11.4

This is the bare minimum to end a copy job (of course, until a later patch adds the ability to start a copy job, this patch doesn't do much in isolation; I've just split the patches to ease the review). This patch intentionally avoids SELinux, lock manager, and audit actions. Also, if libvirtd restarts at the exact moment that a 'block-job-complete' is in flight, the proposed proper way to detect the outcome of that would be with some additional query commands for qemu 1.3 (RHEL 6.3 is out of luck). This patch is enough to test the common case of success when used correctly, while saving the subtleties of proper cleanup for worst-case errors for later. When a mirror job is started, cancelling the job safely reverts back to the source disk, regardless of whether the destination is in phase 1 (streaming, in which case the destination is worthless) or phase 2 (mirroring, in which case the destination is synced up to the source at the time of the cancel). Our existing code does just fine in either phase, other than some bookkeeping cleanup; this implements live block copy. Pivoting the job requires the qemu 1.3 'block-job-complete' (safe) or the RHEL 6.3 '__com.redhat_drive-reopen' command (where failure of the command is potentially catastrophic to the domain, since it rips out the old disk before attempting to open the new one). Ideas for future enhancements via new flags: Since qemu 1.3 is safer than RHEL 6.3, it may be worth adding a VIR_DOMAIN_REBASE_COPY_ATOMIC flag that fails up front if we detect an older qemu with the risky pivot operation. Interesting side note: while snapshot-create --disk-only creates a copy of the disk at a point in time by moving the domain on to a new file (the copy is the file now in the just-extended backing chain), blockjob --abort of a copy job creates a copy of the disk while keeping the domain on the original file. There may be potential improvements to the snapshot code to exploit block copy over multiple disks all at one point in time. And, if 'block-job-cancel' were made part of 'transaction', you could copy multiple disks at the same point in time without pausing the domain. This also implies we may want to add a --quiesce flag to virDomainBlockJobAbort, so that when breaking a mirror (whether by cancel or pivot), the side of the mirror that we are abandoning is at least in a stable state with regards to guest I/O. * src/qemu/qemu_driver.c (qemuDomainBlockJobAbort): Accept new flag. (qemuDomainBlockPivot): New helper function. (qemuDomainBlockJobImpl): Implement it. --- src/qemu/qemu_driver.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8f42f2..df581b4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12607,6 +12607,87 @@ cleanup: 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 + * either success or failure (although there are some forms of + * catastrophic failure that will leave the VM unusable). */ +static int +qemuDomainBlockPivot(struct qemud_driver *driver, virDomainObjPtr vm, + const char *device, virDomainDiskDefPtr disk) +{ + int ret = -1; + qemuDomainObjPrivatePtr priv = vm->privateData; + virDomainBlockJobInfo info; + bool reopen = qemuCapsGet(priv->caps, QEMU_CAPS_DRIVE_REOPEN); + + /* Probe the status, if needed. */ + if (!disk->mirroring) { + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, 0, &info, + BLOCK_JOB_INFO, true); + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + if (ret == 1 && info.cur == info.end && + info.type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) + disk->mirroring = true; + } + + if (!disk->mirroring) { + virReportError(VIR_ERR_BLOCK_COPY_ACTIVE, + _("disk '%s' not ready for pivot yet"), + disk->dst); + goto cleanup; + } + + /* Attempt the pivot. */ + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror, + disk->mirrorFormat, reopen); + qemuDomainObjExitMonitorWithDriver(driver, vm); + + /* Note that RHEL 6.3 'drive-reopen' has the remote risk of a + * catastrophic failure, where the it fails but can't recover by + * reopening the source. Not much we can do about it. qemu 1.3 + * 'block-job-complete' is safer by design. */ + + if (ret == 0) { + /* XXX We want to revoke security labels and disk lease, as + * well as audit that revocation, before dropping the original + * source. But it gets tricky if both source and mirror share + * common backing files (we want to only revoke the non-shared + * portion of the chain, and is made more difficult by the + * fact that we aren't tracking the full chain ourselves; so + * for now, we leak the access to the original. */ + VIR_FREE(disk->src); + VIR_FREE(disk->driverType); + disk->src = disk->mirror; + disk->driverType = disk->mirrorFormat; + disk->mirror = NULL; + disk->mirrorFormat = NULL; + disk->mirroring = false; + } else { + /* On failure, qemu abandons the mirror, and attempts to + * revert back to the source disk. Hopefully it was able to + * reopen things. */ + /* XXX should we be parsing the exact qemu error, or calling + * 'query-block', to see what state we really got left in + * before killing the mirroring job? And just as on the + * success case, there's security labeling to worry about. */ + VIR_FREE(disk->mirror); + VIR_FREE(disk->mirrorFormat); + disk->mirroring = false; + } + +cleanup: + return ret; +} + static int qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, virDomainBlockJobInfoPtr info, @@ -12667,6 +12748,14 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, disk->dst); goto cleanup; } + if (mode == BLOCK_JOB_ABORT && + (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT) && + !(async && disk->mirror)) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("pivot of disk '%s' requires an active copy job"), + disk->dst); + goto cleanup; + } if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -12677,6 +12766,12 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, goto endjob; } + if (disk->mirror && mode == BLOCK_JOB_ABORT && + (flags & VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT)) { + ret = qemuDomainBlockPivot(driver, vm, device, disk); + goto endjob; + } + qemuDomainObjEnterMonitorWithDriver(driver, vm); /* XXX - libvirt should really be tracking the backing file chain * itself, and validating that base is on the chain, rather than @@ -12693,6 +12788,15 @@ qemuDomainBlockJobImpl(virDomainPtr dom, const char *path, const char *base, info->cur == info->end && info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) disk->mirroring = true; + /* A successful block job cancelation stops any mirroring. */ + if (mode == BLOCK_JOB_ABORT && disk->mirror) { + /* XXX We should also revoke security labels and disk lease on + * the mirror, and audit that fact, before dropping things. */ + VIR_FREE(disk->mirror); + VIR_FREE(disk->mirrorFormat); + disk->mirroring = false; + } + /* With synchronous block cancel, we must synthesize an event, and * we silently ignore the ABORT_ASYNC flag. With asynchronous * block cancel, the event will come from qemu, but without the @@ -12757,7 +12861,8 @@ cleanup: static int qemuDomainBlockJobAbort(virDomainPtr dom, const char *path, unsigned int flags) { - virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC | + VIR_DOMAIN_BLOCK_JOB_ABORT_PIVOT, -1); return qemuDomainBlockJobImpl(dom, path, NULL, 0, NULL, BLOCK_JOB_ABORT, flags); } -- 1.7.11.4

This copies some of the checks from snapshots regarding testing when a file already exists. In the process, I noticed snapshots had hard-to-read logic, as well as a missing sanity check: REUSE_EXT should require the destination to already be present. * src/qemu/qemu_driver.c (qemuDomainBlockRebase): Allow REUSE_EXT flag. (qemuDomainBlockCopy): Wire up flag, and add some sanity checks. (qemuDomainSnapshotDiskPrepare): Require destination on REUSE_EXT. --- src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index da34f14..0afb1c8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10641,8 +10641,13 @@ qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def, _("unable to stat for disk %s: %s"), disk->name, disk->file); goto cleanup; + } else if (allow_reuse) { + virReportSystemError(errno, + _("missing existing file for disk %s: %s"), + disk->name, disk->file); + goto cleanup; } - } else if (!(S_ISBLK(st.st_mode) || !st.st_size || allow_reuse)) { + } else if (!S_ISBLK(st.st_mode) && st.st_size && !allow_reuse) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("external snapshot file for disk %s already " "exists and is not a block device: %s"), @@ -12930,9 +12935,11 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *path, int ret = -1; int idx; bool reopen; + struct stat st; /* Preliminaries: find the disk we are editing, sanity checks */ - virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW, -1); + virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT, -1); qemuDriverLock(driver); virUUIDFormat(dom->uuid, uuidstr); @@ -12987,12 +12994,42 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *path, goto endjob; } + /* XXX this is pessimistic; we could use 'query-block' or even + * keep track of the backing chain ourselves, rather than assuming + * that all non-raw source files have a current backing image */ + if ((flags & VIR_DOMAIN_BLOCK_REBASE_SHALLOW) && + STREQ_NULLABLE(format, "raw") && + STRNEQ_NULLABLE(disk->driverType, "raw")) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("raw shallow copy of non-raw disk '%s' not possible"), + disk->dst); + goto endjob; + } + /* Prepare the destination file. */ + if (stat(dest, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, _("unable to stat for disk %s: %s"), + disk->dst, dest); + goto endjob; + } else if (flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT) { + virReportSystemError(errno, + _("missing destination file for disk %s: %s"), + disk->dst, dest); + goto endjob; + } + } else if (!S_ISBLK(st.st_mode) && st.st_size && + !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external destination file for disk %s already " + "exists and is not a block device: %s"), + disk->dst, dest); + goto endjob; + } + /* XXX We also need to add security labeling, lock manager lease, - * and auditing of those events, as well as to support reuse of - * existing images, including probing the existing format of an - * existing image. */ - if (!format) + * and auditing of those events. */ + if (!format && !(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) format = disk->driverType; if ((format && !(disk->mirrorFormat = strdup(format))) || !(disk->mirror = strdup(dest))) { @@ -13029,6 +13066,7 @@ qemuDomainBlockRebase(virDomainPtr dom, const char *path, const char *base, unsigned long bandwidth, unsigned int flags) { virCheckFlags(VIR_DOMAIN_BLOCK_REBASE_SHALLOW | + VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT | VIR_DOMAIN_BLOCK_REBASE_COPY | VIR_DOMAIN_BLOCK_REBASE_COPY_RAW, -1); -- 1.7.11.4

When using block copy to pivot over to a new chain, and the user passes --reuse-ext, it could be that the user has already transferred the entire backing chain, and that the external file being reused has a backing chain that needs SELinux labeling. https://bugzilla.redhat.com/show_bug.cgi?id=856247 * src/qemu/qemu_driver.c (qemuDomainBlockCopy): Don't bypass labeling when reusing external file. --- This patch is new compared to the v6 posting. I'm actually planning on squashing this into patch 8/9 before pushing, but posting for separate review makes it easier to track how this is being backported to RHEL 6.3. src/qemu/qemu_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b9014c3..246969e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13060,7 +13060,11 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *path, origsrc = disk->src; disk->src = (char *) dest; origdriver = disk->driverType; - disk->driverType = (char *) "raw"; /* Don't want to probe backing files */ + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { + /* Don't want to probe backing files unless the chain is + * coming from the external use */ + disk->driverType = (char *) "raw"; + } if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) goto endjob; -- 1.7.11.4

When using block copy to pivot over to a new chain, the backing files for the new chain might still need labeling (particularly if the user passes --reuse-ext with a relative backing file name). Relabeling a file that is already labeled won't hurt, so this just labels the entire chain at the point of the pivot. https://bugzilla.redhat.com/show_bug.cgi?id=856247 * src/qemu/qemu_driver.c (qemuDomainBlockPivot): Relabel chain before asking qemu to pivot. --- Diff from v7.5: relabel the new disk chain, not the old chain src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8201c9d..828ad6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12596,6 +12596,9 @@ qemuDomainBlockPivot(virConnectPtr conn, virDomainBlockJobInfo info; bool reopen = qemuCapsGet(priv->caps, QEMU_CAPS_DRIVE_REOPEN); bool resume = false; + virCgroupPtr cgroup = NULL; + char *oldsrc = NULL; + char *olddriver = NULL; /* Probe the status, if needed. */ if (!disk->mirroring) { @@ -12643,6 +12646,30 @@ qemuDomainBlockPivot(virConnectPtr conn, } } + /* We previously labeled only the top-level image; but if the + * image includes a relative backing file, the pivot may result in + * qemu needing to open the entire backing chain, so we need to + * label the entire chain. This action is safe even if the + * backing chain has already been labeled. */ + if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES) && + virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unable to find cgroup for %s"), + vm->def->name); + goto cleanup; + } + oldsrc = disk->src; + olddriver = disk->driverType; + disk->src = disk->mirror; + disk->driverType = disk->mirrorFormat; + if ((cgroup && qemuSetupDiskCgroup(driver, vm, cgroup, disk) < 0) || + virSecurityManagerSetImageLabel(driver->securityManager, vm->def, + disk) < 0) { + disk->src = oldsrc; + disk->driverType = olddriver; + goto cleanup; + } + /* Attempt the pivot. */ qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorDrivePivot(priv->mon, device, disk->mirror, @@ -12662,10 +12689,8 @@ qemuDomainBlockPivot(virConnectPtr conn, * portion of the chain, and is made more difficult by the * fact that we aren't tracking the full chain ourselves; so * for now, we leak the access to the original. */ - VIR_FREE(disk->src); - VIR_FREE(disk->driverType); - disk->src = disk->mirror; - disk->driverType = disk->mirrorFormat; + VIR_FREE(oldsrc); + VIR_FREE(olddriver); disk->mirror = NULL; disk->mirrorFormat = NULL; disk->mirroring = false; @@ -12677,12 +12702,16 @@ qemuDomainBlockPivot(virConnectPtr conn, * 'query-block', to see what state we really got left in * before killing the mirroring job? And just as on the * success case, there's security labeling to worry about. */ + disk->src = oldsrc; + disk->driverType = olddriver; VIR_FREE(disk->mirror); VIR_FREE(disk->mirrorFormat); disk->mirroring = false; } cleanup: + if (cgroup) + virCgroupFree(&cgroup); if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, -- 1.7.11.4

SIGINT hasn't been blocked, which could lead to losing it somewhere in virDomainGetBlockJobInfo and not aborting the job. --- tools/virsh-domain.c | 24 ++++++++++++++++++------ 1 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 1df0872..01550c7 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1308,7 +1308,7 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; - sigset_t sigmask; + sigset_t sigmask, oldsigmask; struct timeval start; struct timeval curr; const char *path = NULL; @@ -1361,7 +1361,11 @@ cmdBlockCommit(vshControl *ctl, const vshCmd *cmd) while (blocking) { virDomainBlockJobInfo info; - int result = virDomainGetBlockJobInfo(dom, path, &info, 0); + int result; + + pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); + result = virDomainGetBlockJobInfo(dom, path, &info, 0); + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); if (result < 0) { vshError(ctl, _("failed to query job for disk %s"), path); @@ -1450,7 +1454,7 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; - sigset_t sigmask; + sigset_t sigmask, oldsigmask; struct timeval start; struct timeval curr; const char *path = NULL; @@ -1507,7 +1511,11 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) while (blocking) { virDomainBlockJobInfo info; - int result = virDomainGetBlockJobInfo(dom, path, &info, 0); + int result; + + pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); + result = virDomainGetBlockJobInfo(dom, path, &info, 0); + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); if (result <= 0) { vshError(ctl, _("failed to query job for disk %s"), path); @@ -1674,7 +1682,7 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) int timeout = 0; struct sigaction sig_action; struct sigaction old_sig_action; - sigset_t sigmask; + sigset_t sigmask,oldsigmask; struct timeval start; struct timeval curr; const char *path = NULL; @@ -1727,7 +1735,11 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd) while (blocking) { virDomainBlockJobInfo info; - int result = virDomainGetBlockJobInfo(dom, path, &info, 0); + int result; + + pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask); + result = virDomainGetBlockJobInfo(dom, path, &info, 0); + pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL); if (result < 0) { vshError(ctl, _("failed to query job for disk %s"), path); -- 1.7.8.6

--- tools/virsh-domain.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 01550c7..1a9097f 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1468,13 +1468,13 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptInt(cmd, "timeout", &timeout) > 0) { if (timeout < 1) { - vshError(ctl, "%s", _("migrate: Invalid timeout")); + vshError(ctl, "%s", _("invalid timeout")); return false; } /* Ensure that we can multiply by 1000 without overflowing. */ if (timeout > INT_MAX / 1000) { - vshError(ctl, "%s", _("migrate: Timeout is too big")); + vshError(ctl, "%s", _("timeout is too big")); return false; } timeout *= 1000; -- 1.7.8.6