[libvirt] [PATCH 0/2] atomic snapshots, round 2
Depends on round 1 v2 being applied first: https://www.redhat.com/archives/libvir-list/2012-March/msg00747.html So far, I've tested that the error handling path works when targeting the qemu of Fedora 16; I still plan to test that the success path works with a self-built qemu using the code that has been accepted for the upcoming qemu 1.1. Eric Blake (2): snapshot: add support for qemu transaction command snapshot: wire up qemu transaction command src/qemu/qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_monitor.c | 33 ++++++++++-- src/qemu/qemu_monitor.h | 4 ++ src/qemu/qemu_monitor_json.c | 82 ++++++++++++++++++++++++------- src/qemu/qemu_monitor_json.h | 2 + 5 files changed, 205 insertions(+), 28 deletions(-) -- 1.7.7.6
QEmu 1.1 is adding a 'transaction' command to the JSON monitor. Each element of a transaction corresponds to a top-level command, with the additional guarantee that the transaction flushes all pending I/O, then guarantees that all actions will be successful as a group or that failure will roll back the state to what it was before the monitor command. The difference between a top-level command: { "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0", ... } } and a transaction: { "execute": "transaction", "arguments": { "actions": [ { "type" "blockdev-snapshot-sync", "data": { "device": "virtio0", ... } } ] } } is just a couple of changed key names and nesting the shorter command inside a JSON array to the longer command. This patch just adds the framework; the next patch will actually use a transaction. * src/qemu/qemu_monitor_json.c (qemuMonitorJSONMakeCommand): Move guts... (qemuMonitorJSONMakeCommandRaw): ...into new helper. Add support for array element. (qemuMonitorJSONTransaction): New command. (qemuMonitorJSONDiskSnapshot): Support use in a transaction. * src/qemu/qemu_monitor_json.h (qemuMonitorJSONDiskSnapshot): Add argument. (qemuMonitorJSONTransaction): New declaration. * src/qemu/qemu_monitor.h (qemuMonitorTransaction): Likewise. (qemuMonitorDiskSnapshot): Add argument. * src/qemu/qemu_monitor.c (qemuMonitorTransaction): New wrapper. (qemuMonitorDiskSnapshot): Pass argument on. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive): Update caller. --- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_monitor.c | 33 ++++++++++++++--- src/qemu/qemu_monitor.h | 4 ++ src/qemu/qemu_monitor_json.c | 82 ++++++++++++++++++++++++++++++++--------- src/qemu/qemu_monitor_json.h | 2 + 5 files changed, 99 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c2c3b92..c3bbc3f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9882,7 +9882,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = NULL; /* create the actual snapshot */ - ret = qemuMonitorDiskSnapshot(priv->mon, device, source); + ret = qemuMonitorDiskSnapshot(priv->mon, NULL, device, source); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 78eb492..1d54249 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2623,12 +2623,13 @@ int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name) * device into a read-only backing file of a new qcow2 image located * at file. */ int -qemuMonitorDiskSnapshot(qemuMonitorPtr mon, const char *device, - const char *file) +qemuMonitorDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, + const char *device, const char *file) { int ret; - VIR_DEBUG("mon=%p, device=%s, file=%s", mon, device, file); + VIR_DEBUG("mon=%p, actions=%p, device=%s, file=%s", + mon, actions, device, file); if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", @@ -2636,10 +2637,32 @@ qemuMonitorDiskSnapshot(qemuMonitorPtr mon, const char *device, return -1; } + if (mon->json) { + ret = qemuMonitorJSONDiskSnapshot(mon, actions, device, file); + } else { + if (actions) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("actions not supported with text monitor")); + return -1; + } + ret = qemuMonitorTextDiskSnapshot(mon, device, file); + } + return ret; +} + +/* Use the transaction QMP command to run atomic snapshot commands. */ +int +qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) +{ + int ret = -1; + + VIR_DEBUG("mon=%p, actions=%p", mon, actions); + if (mon->json) - ret = qemuMonitorJSONDiskSnapshot(mon, device, file); + ret = qemuMonitorJSONTransaction(mon, actions); else - ret = qemuMonitorTextDiskSnapshot(mon, device, file); + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("transaction requires JSON monitor")); return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 654d9bd..7e91951 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -31,6 +31,7 @@ # include "qemu_conf.h" # include "bitmap.h" # include "virhash.h" +# include "json.h" typedef struct _qemuMonitor qemuMonitor; typedef qemuMonitor *qemuMonitorPtr; @@ -492,8 +493,11 @@ int qemuMonitorLoadSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorDeleteSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorDiskSnapshot(qemuMonitorPtr mon, + virJSONValuePtr actions, const char *device, const char *file); +int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuMonitorArbitraryCommand(qemuMonitorPtr mon, const char *cmd, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4dd6924..ba07e84 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -369,9 +369,10 @@ qemuMonitorJSONHasError(virJSONValuePtr reply, return STREQ(klass, thisklass); } +/* Top-level commands and nested transaction list elements share a + * common structure for everything except the dictionary names. */ static virJSONValuePtr ATTRIBUTE_SENTINEL -qemuMonitorJSONMakeCommand(const char *cmdname, - ...) +qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) { virJSONValuePtr obj; virJSONValuePtr jargs = NULL; @@ -383,7 +384,8 @@ qemuMonitorJSONMakeCommand(const char *cmdname, if (!(obj = virJSONValueNewObject())) goto no_memory; - if (virJSONValueObjectAppendString(obj, "execute", cmdname) < 0) + if (virJSONValueObjectAppendString(obj, wrap ? "type" : "execute", + cmdname) < 0) goto no_memory; while ((key = va_arg(args, char *)) != NULL) { @@ -405,8 +407,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname, !(jargs = virJSONValueNewObject())) goto no_memory; - /* This doesn't supports maps/arrays. This hasn't - * proved to be a problem..... yet :-) */ + /* This doesn't support maps, but no command uses those. */ switch (type) { case 's': { char *val = va_arg(args, char *); @@ -444,6 +445,10 @@ qemuMonitorJSONMakeCommand(const char *cmdname, case 'n': { ret = virJSONValueObjectAppendNull(jargs, key); } break; + case 'a': { + virJSONValuePtr val = va_arg(args, virJSONValuePtr); + ret = virJSONValueObjectAppend(jargs, key, val); + } break; default: qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unsupported data type '%c' for arg '%s'"), type, key - 2); @@ -454,7 +459,7 @@ qemuMonitorJSONMakeCommand(const char *cmdname, } if (jargs && - virJSONValueObjectAppend(obj, "arguments", jargs) < 0) + virJSONValueObjectAppend(obj, wrap ? "data" : "arguments", jargs) < 0) goto no_memory; va_end(args); @@ -470,6 +475,8 @@ error: return NULL; } +#define qemuMonitorJSONMakeCommand(cmdname, ...) \ + qemuMonitorJSONMakeCommandRaw(false, cmdname, __VA_ARGS__) static void qemuFreeKeywords(int nkeywords, char **keywords, char **values) @@ -3052,30 +3059,69 @@ cleanup: } int -qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, const char *device, - const char *file) +qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, virJSONValuePtr actions, + const char *device, const char *file) { int ret; virJSONValuePtr cmd; virJSONValuePtr reply = NULL; - cmd = qemuMonitorJSONMakeCommand("blockdev-snapshot-sync", - "s:device", device, - "s:snapshot-file", file, - NULL); + cmd = qemuMonitorJSONMakeCommandRaw(actions != NULL, + "blockdev-snapshot-sync", + "s:device", device, + "s:snapshot-file", file, + NULL); if (!cmd) return -1; - if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + if (actions) { + if (virJSONValueArrayAppend(actions, cmd) < 0) { + virReportOOMError(); + ret = -1; + } else { + ret = 0; + cmd = NULL; + } + } else { + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) goto cleanup; - if (qemuMonitorJSONHasError(reply, "CommandNotFound") && - qemuMonitorCheckHMP(mon, "snapshot_blkdev")) { - VIR_DEBUG("blockdev-snapshot-sync command not found, trying HMP"); - ret = qemuMonitorTextDiskSnapshot(mon, device, file); - goto cleanup; + if (qemuMonitorJSONHasError(reply, "CommandNotFound") && + qemuMonitorCheckHMP(mon, "snapshot_blkdev")) { + VIR_DEBUG("blockdev-snapshot-sync command not found, trying HMP"); + ret = qemuMonitorTextDiskSnapshot(mon, device, file); + goto cleanup; + } + + ret = qemuMonitorJSONCheckError(cmd, reply); + } + +cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + return ret; +} + +/* Note that this call frees actions regardless of whether the call + * succeeds. */ +int +qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions) +{ + int ret; + virJSONValuePtr cmd; + virJSONValuePtr reply = NULL; + + cmd = qemuMonitorJSONMakeCommand("transaction", + "a:actions", actions, + NULL); + if (!cmd) { + virJSONValueFree(actions); + return -1; } + if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0) + goto cleanup; + ret = qemuMonitorJSONCheckError(cmd, reply); cleanup: diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 04e9d86..e127870 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -226,8 +226,10 @@ int qemuMonitorJSONLoadSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorJSONDeleteSnapshot(qemuMonitorPtr mon, const char *name); int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon, + virJSONValuePtr actions, const char *device, const char *file); +int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions); int qemuMonitorJSONArbitraryCommand(qemuMonitorPtr mon, const char *cmd_str, -- 1.7.7.6
On 03/17/2012 10:27 PM, Eric Blake wrote:
QEmu 1.1 is adding a 'transaction' command to the JSON monitor. Each element of a transaction corresponds to a top-level command, with the additional guarantee that the transaction flushes all pending I/O, then guarantees that all actions will be successful as a group or that failure will roll back the state to what it was before the monitor command. The difference between a top-level command:
{ "execute": "blockdev-snapshot-sync", "arguments": { "device": "virtio0", ... } }
and a transaction:
{ "execute": "transaction", "arguments": { "actions": [ { "type" "blockdev-snapshot-sync", "data":
Shouldn't there be a colon between "type" and "block...."?
{ "device": "virtio0", ... } } ] } }
is just a couple of changed key names and nesting the shorter command inside a JSON array to the longer command. This patch just adds the framework; the next patch will actually use a transaction.
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4dd6924..ba07e84 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -383,7 +384,8 @@ qemuMonitorJSONMakeCommand(const char *cmdname, if (!(obj = virJSONValueNewObject())) goto no_memory;
- if (virJSONValueObjectAppendString(obj, "execute", cmdname)< 0) + if (virJSONValueObjectAppendString(obj, wrap ? "type" : "execute", + cmdname)< 0) goto no_memory;
Hopefuly QEmu won't add another wrapping type.
while ((key = va_arg(args, char *)) != NULL) {
ACK, Peter
The hardest part about adding transactions is not using the new monitor command, but undoing the partial changes we made prior to a failed transaction. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use transaction when available. (qemuDomainSnapshotUndoSingleDiskActive): New function. (qemuDomainSnapshotCreateSingleDiskActive): Pass through actions. (qemuDomainSnapshotCreateXML): Adjust caller. --- src/qemu/qemu_driver.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 107 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c3bbc3f..2bc8d5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9822,7 +9822,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk, - virDomainDiskDefPtr persistDisk) + virDomainDiskDefPtr persistDisk, + virJSONValuePtr actions) { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; @@ -9882,7 +9883,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = NULL; /* create the actual snapshot */ - ret = qemuMonitorDiskSnapshot(priv->mon, NULL, device, source); + ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) goto cleanup; @@ -9923,6 +9924,69 @@ cleanup: return ret; } +/* The domain is expected to hold monitor lock. This is the + * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called + * only on a failed transaction. */ +static void +qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDiskDefPtr origdisk, + virDomainDiskDefPtr disk, + virDomainDiskDefPtr persistDisk, + bool need_unlink) +{ + char *source = NULL; + char *driverType = NULL; + char *persistSource = NULL; + char *persistDriverType = NULL; + struct stat st; + + if (!(source = strdup(origdisk->src)) || + (origdisk->driverType && + !(driverType = strdup(origdisk->driverType))) || + (persistDisk && + (!(persistSource = strdup(source)) || + (driverType && !(persistDriverType = strdup(driverType)))))) { + virReportOOMError(); + goto cleanup; + } + + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, disk) < 0) + VIR_WARN("Unable to restore security label on %s", disk->src); + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + VIR_WARN("Unable to release lock on %s", source); + if (need_unlink && stat(disk->src, &st) == 0 + && st.st_size == 0 && S_ISREG(st.st_mode) && unlink(disk->src) < 0) + VIR_WARN("Unable to release lock on %s", disk->src); + + /* Update vm in place to match changes. */ + VIR_FREE(disk->src); + disk->src = source; + source = NULL; + VIR_FREE(disk->driverType); + if (driverType) { + disk->driverType = driverType; + driverType = NULL; + } + if (persistDisk) { + VIR_FREE(persistDisk->src); + persistDisk->src = persistSource; + persistSource = NULL; + VIR_FREE(persistDisk->driverType); + if (persistDriverType) { + persistDisk->driverType = persistDriverType; + persistDriverType = NULL; + } + } + +cleanup: + VIR_FREE(source); + VIR_FREE(driverType); + VIR_FREE(persistSource); + VIR_FREE(persistDriverType); +} + /* The domain is expected to be locked and active. */ static int qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, @@ -9932,6 +9996,8 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, unsigned int flags) { virDomainObjPtr vm = *vmptr; + qemuDomainObjPrivatePtr priv = vm->privateData; + virJSONValuePtr actions = NULL; bool resume = false; int ret = -1; int i; @@ -9981,9 +10047,17 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, goto cleanup; } } + if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION)) { + actions = virJSONValueNewArray(); + if (!actions) { + virReportOOMError(); + goto cleanup; + } + } /* No way to roll back if first disk succeeds but later disks - * fail. Based on earlier qemuDomainSnapshotDiskPrepare, all + * fail, unless we have transaction support. + * Based on earlier qemuDomainSnapshotDiskPrepare, all * disks in this list are now either SNAPSHOT_NO, or * SNAPSHOT_EXTERNAL with a valid file name and qcow2 format. */ qemuDomainObjEnterMonitorWithDriver(driver, vm); @@ -10005,10 +10079,37 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], vm->def->disks[i], - persistDisk); + persistDisk, actions); if (ret < 0) break; } + if (actions) { + if (ret == 0) + ret = qemuMonitorTransaction(priv->mon, actions); + if (ret < 0) { + /* Transaction failed; undo the changes to vm. */ + bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT); + while (--i >= 0) { + virDomainDiskDefPtr persistDisk = NULL; + + if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + continue; + if (vm->newDef) { + int indx = virDomainDiskIndexByName(vm->newDef, + vm->def->disks[i]->dst, + false); + if (indx >= 0) + persistDisk = vm->newDef->disks[indx]; + } + + qemuDomainSnapshotUndoSingleDiskActive(driver, vm, + snap->def->dom->disks[i], + vm->def->disks[i], + persistDisk, + need_unlink); + } + } + } qemuDomainObjExitMonitorWithDriver(driver, vm); if (ret < 0) goto cleanup; @@ -10042,7 +10143,8 @@ cleanup: } } - if (vm) { + if (vm && (ret == 0 || + !qemuCapsGet(priv->qemuCaps, QEMU_CAPS_TRANSACTION))) { if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0 || (persist && virDomainSaveConfig(driver->configDir, vm->newDef) < 0)) -- 1.7.7.6
On 03/17/2012 10:27 PM, Eric Blake wrote:
The hardest part about adding transactions is not using the new monitor command, but undoing the partial changes we made prior to a failed transaction.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive): Use transaction when available. (qemuDomainSnapshotUndoSingleDiskActive): New function. (qemuDomainSnapshotCreateSingleDiskActive): Pass through actions. (qemuDomainSnapshotCreateXML): Adjust caller. --- src/qemu/qemu_driver.c | 112 +++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 107 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c3bbc3f..2bc8d5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9822,7 +9822,8 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk, - virDomainDiskDefPtr persistDisk) + virDomainDiskDefPtr persistDisk, + virJSONValuePtr actions) { qemuDomainObjPrivatePtr priv = vm->privateData; char *device = NULL; @@ -9882,7 +9883,7 @@ qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, origdriver = NULL;
/* create the actual snapshot */ - ret = qemuMonitorDiskSnapshot(priv->mon, NULL, device, source); + ret = qemuMonitorDiskSnapshot(priv->mon, actions, device, source); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret>= 0); if (ret< 0) goto cleanup; @@ -9923,6 +9924,69 @@ cleanup: return ret; }
+/* The domain is expected to hold monitor lock. This is the + * counterpart to qemuDomainSnapshotCreateSingleDiskActive, called + * only on a failed transaction. */ +static void +qemuDomainSnapshotUndoSingleDiskActive(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainDiskDefPtr origdisk, + virDomainDiskDefPtr disk, + virDomainDiskDefPtr persistDisk, + bool need_unlink) +{ + char *source = NULL; + char *driverType = NULL; + char *persistSource = NULL; + char *persistDriverType = NULL; + struct stat st; + + if (!(source = strdup(origdisk->src)) || + (origdisk->driverType&& + !(driverType = strdup(origdisk->driverType))) || + (persistDisk&& + (!(persistSource = strdup(source)) || + (driverType&& !(persistDriverType = strdup(driverType)))))) { + virReportOOMError(); + goto cleanup; + } + + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, disk)< 0) + VIR_WARN("Unable to restore security label on %s", disk->src); + if (virDomainLockDiskDetach(driver->lockManager, vm, disk)< 0) + VIR_WARN("Unable to release lock on %s", source); + if (need_unlink&& stat(disk->src,&st) == 0 +&& st.st_size == 0&& S_ISREG(st.st_mode)&& unlink(disk->src)< 0) + VIR_WARN("Unable to release lock on %s", disk->src);
Is this second warning correct? It looks to me like if you are unlinking the disk source file, but the error message states that lock release failed.
+ + /* Update vm in place to match changes. */ + VIR_FREE(disk->src); + disk->src = source; + source = NULL; + VIR_FREE(disk->driverType); + if (driverType) { + disk->driverType = driverType; + driverType = NULL; + } + if (persistDisk) { + VIR_FREE(persistDisk->src); + persistDisk->src = persistSource; + persistSource = NULL; + VIR_FREE(persistDisk->driverType); + if (persistDriverType) { + persistDisk->driverType = persistDriverType; + persistDriverType = NULL; + } + } + +cleanup: + VIR_FREE(source); + VIR_FREE(driverType); + VIR_FREE(persistSource); + VIR_FREE(persistDriverType); +} +
I don't have many experience working with the locking and security code, but this funcion looks as it's doing what it should. I'd feel more confident if somebody other would look at this part. ACK to the rest, and a not-so-confident ACK of the marked part if my question gets cleared. Peter
On 03/22/2012 08:47 AM, Peter Krempa wrote:
On 03/17/2012 10:27 PM, Eric Blake wrote:
The hardest part about adding transactions is not using the new monitor command, but undoing the partial changes we made prior to a failed transaction.
+ if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, disk)< 0) + VIR_WARN("Unable to restore security label on %s", disk->src); + if (virDomainLockDiskDetach(driver->lockManager, vm, disk)< 0) + VIR_WARN("Unable to release lock on %s", source); + if (need_unlink&& stat(disk->src,&st) == 0 +&& st.st_size == 0&& S_ISREG(st.st_mode)&& unlink(disk->src)< 0) + VIR_WARN("Unable to release lock on %s", disk->src);
Is this second warning correct? It looks to me like if you are unlinking the disk source file, but the error message states that lock release failed.
Looks like too much copy and paste on my part. I'll change it to _("Unable to delete just-created file %s").
I don't have many experience working with the locking and security code, but this funcion looks as it's doing what it should. I'd feel more confident if somebody other would look at this part.
ACK to the rest, and a not-so-confident ACK of the marked part if my question gets cleared.
Thanks for reviewing. As with round 1, I will delay pushing these until I have finished rounds 4 and 5 for the complete mirrored storage migration solution, in case I come up with any other last-minute tweaks. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/22/2012 08:53 AM, Eric Blake wrote:
On 03/22/2012 08:47 AM, Peter Krempa wrote:
On 03/17/2012 10:27 PM, Eric Blake wrote:
The hardest part about adding transactions is not using the new monitor command, but undoing the partial changes we made prior to a failed transaction.
I don't have many experience working with the locking and security code, but this funcion looks as it's doing what it should. I'd feel more confident if somebody other would look at this part.
ACK to the rest, and a not-so-confident ACK of the marked part if my question gets cleared.
I've folded in the mentioned fixes,
Thanks for reviewing. As with round 1, I will delay pushing these until I have finished rounds 4 and 5 for the complete mirrored storage migration solution, in case I come up with any other last-minute tweaks.
and pushed round 2 now. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa