[libvirt] [PATCH 0/5] qemu: Fix job handling in snapshot code
The snapshot code had some issues with job handling. This is an attempt to do it right. Jincheng Miao (1): qemu: snapshot: Acquire job earlier on snapshot revert/delete Peter Krempa (4): qemu: Rename DEFAULT_JOB_MASK to QEMU_DEFAULT_JOB_MASK qemu: snapshot: Fix job handling when creating snapshots qemu: snapshot: Fix snapshot function header formatting qemu: snapshot: Simplify error paths src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 317 +++++++++++++++++++++------------------------- src/qemu/qemu_migration.c | 6 +- 4 files changed, 151 insertions(+), 176 deletions(-) -- 2.0.2
Be consistend with naming of private defines. Also line up code correctly in few places where the macro is used. --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e9506e0..543c064 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -162,7 +162,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv) job->asyncJob = QEMU_ASYNC_JOB_NONE; job->asyncOwner = 0; job->phase = 0; - job->mask = DEFAULT_JOB_MASK; + job->mask = QEMU_DEFAULT_JOB_MASK; job->start = 0; job->dump_memory_only = false; job->asyncAbort = false; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 8736889..f959664 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -54,7 +54,7 @@ # endif # define JOB_MASK(job) (1 << (job - 1)) -# define DEFAULT_JOB_MASK \ +# define QEMU_DEFAULT_JOB_MASK \ (JOB_MASK(QEMU_JOB_QUERY) | \ JOB_MASK(QEMU_JOB_DESTROY) | \ JOB_MASK(QEMU_JOB_ABORT)) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 239a300..dd831b0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13207,9 +13207,9 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, goto endjob; /* allow the migration job to be cancelled or the domain to be paused */ - qemuDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK | - JOB_MASK(QEMU_JOB_SUSPEND) | - JOB_MASK(QEMU_JOB_MIGRATION_OP)); + qemuDomainObjSetAsyncJobMask(vm, QEMU_DEFAULT_JOB_MASK | + JOB_MASK(QEMU_JOB_SUSPEND) | + JOB_MASK(QEMU_JOB_MIGRATION_OP)); cfg = virQEMUDriverGetConfig(driver); if (cfg->snapshotImageFormat) { @@ -13241,7 +13241,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, memory_unlink = true; /* forbid any further manipulation */ - qemuDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK); + qemuDomainObjSetAsyncJobMask(vm, QEMU_DEFAULT_JOB_MASK); } /* now the domain is now paused if: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 9cfb77e..0954751 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4907,9 +4907,9 @@ qemuMigrationJobStart(virQEMUDriverPtr driver, if (job == QEMU_ASYNC_JOB_MIGRATION_IN) { qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); } else { - qemuDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK | - JOB_MASK(QEMU_JOB_SUSPEND) | - JOB_MASK(QEMU_JOB_MIGRATION_OP)); + qemuDomainObjSetAsyncJobMask(vm, QEMU_DEFAULT_JOB_MASK | + JOB_MASK(QEMU_JOB_SUSPEND) | + JOB_MASK(QEMU_JOB_MIGRATION_OP)); } priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; -- 2.0.2
On 09/03/2014 07:53 AM, Peter Krempa wrote:
Be consistend with naming of private defines. Also line up code
s/consistend/consistent/
correctly in few places where the macro is used. --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 6 +++--- 4 files changed, 9 insertions(+), 9 deletions(-)
# define JOB_MASK(job) (1 << (job - 1)) -# define DEFAULT_JOB_MASK \ +# define QEMU_DEFAULT_JOB_MASK \ (JOB_MASK(QEMU_JOB_QUERY) | \ JOB_MASK(QEMU_JOB_DESTROY) | \ JOB_MASK(QEMU_JOB_ABORT))
I think your naming choice is okay, but it might look slightly better as QEMU_JOB_DEFAULT_MASK, so that it shares the same prefix.
+++ b/src/qemu/qemu_migration.c @@ -4907,9 +4907,9 @@ qemuMigrationJobStart(virQEMUDriverPtr driver, if (job == QEMU_ASYNC_JOB_MIGRATION_IN) { qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); } else { - qemuDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK | - JOB_MASK(QEMU_JOB_SUSPEND) | - JOB_MASK(QEMU_JOB_MIGRATION_OP)); + qemuDomainObjSetAsyncJobMask(vm, QEMU_DEFAULT_JOB_MASK | + JOB_MASK(QEMU_JOB_SUSPEND) | + JOB_MASK(QEMU_JOB_MIGRATION_OP)); }
At least in emacs, I find that lining up a split second argument by using TAB to trigger automatic indentation is easier when done with extra (), as in: qemuDomainObjSetAsyncJobMask(vm, (QEMU_DEFAULT_JOB_MASK | JOB_MASK(QEMU_JOB_SUSPEND) | JOB_MASK(QEMU_JOB_MIGRATION_OP))); But that's purely cosmetic. Whether or not you make those changes, ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Creating snapshots modifies the domain state. Currently we wouldn't enter the job for certain operations although they would modify the state. Refactor job handling so that everything is covered by an async job. --- src/qemu/qemu_driver.c | 176 ++++++++++++++++++++++--------------------------- 1 file changed, 78 insertions(+), 98 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd831b0..9f200f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12355,32 +12355,22 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver, static int qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, virQEMUDriverPtr driver, - virDomainObjPtr *vmptr, + virDomainObjPtr vm, virDomainSnapshotObjPtr snap, unsigned int flags) { - virDomainObjPtr vm = *vmptr; qemuDomainObjPrivatePtr priv = vm->privateData; virObjectEventPtr event = NULL; bool resume = false; int ret = -1; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - return -1; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } - if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { /* savevm monitor command pauses the domain emitting an event which * confuses libvirt since it's not notified when qemu resumes the * domain. Thus we stop and start CPUs ourselves. */ if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_SNAPSHOT) < 0) goto cleanup; resume = true; @@ -12391,7 +12381,12 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, } } - qemuDomainObjEnterMonitor(driver, vm); + if (qemuDomainObjEnterMonitorAsync(driver, vm, + QEMU_ASYNC_JOB_SNAPSHOT) < 0) { + resume = false; + goto cleanup; + } + ret = qemuMonitorCreateSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) @@ -12402,18 +12397,14 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); - /* We already filtered the _HALT flag for persistent domains - * only, so this end job never drops the last reference. */ - ignore_value(qemuDomainObjEndJob(driver, vm)); resume = false; - vm = NULL; } cleanup: if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, - QEMU_ASYNC_JOB_NONE) < 0) { + QEMU_ASYNC_JOB_SNAPSHOT) < 0) { event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -12423,14 +12414,6 @@ qemuDomainSnapshotCreateActiveInternal(virConnectPtr conn, } } - endjob: - if (vm && !qemuDomainObjEndJob(driver, vm)) { - /* Only possible if a transient vm quit while our locks were down, - * in which case we don't want to save snapshot metadata. */ - *vmptr = NULL; - ret = -1; - } - if (event) qemuDomainEventQueue(driver, event); @@ -13132,13 +13115,13 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, static int qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, virQEMUDriverPtr driver, - virDomainObjPtr *vmptr, + virDomainObjPtr vm, virDomainSnapshotObjPtr snap, unsigned int flags) { + virObjectEventPtr event; bool resume = false; int ret = -1; - virDomainObjPtr vm = *vmptr; qemuDomainObjPrivatePtr priv = vm->privateData; char *xml = NULL; bool memory = snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; @@ -13150,9 +13133,6 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, virQEMUDriverConfigPtr cfg = NULL; int compressed = QEMU_SAVE_FORMAT_RAW; - if (qemuDomainObjBeginAsyncJob(driver, vm, QEMU_ASYNC_JOB_SNAPSHOT) < 0) - goto cleanup; - /* If quiesce was requested, then issue a freeze command, and a * counterpart thaw command when it is actually sent to agent. * The command will fail if the guest is paused or the guest agent @@ -13163,7 +13143,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, /* the helper reported the error */ if (freeze == -2) thaw = -1; /* the command is sent but agent failed */ - goto endjob; + goto cleanup; } thaw = 1; } @@ -13190,12 +13170,12 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, (!memory && atomic && !transaction)) { if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SNAPSHOT, QEMU_ASYNC_JOB_SNAPSHOT) < 0) - goto endjob; + goto cleanup; if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("guest unexpectedly quit")); - goto endjob; + goto cleanup; } } } @@ -13204,7 +13184,7 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, if (memory) { /* check if migration is possible */ if (!qemuMigrationIsAllowed(driver, vm, vm->def, false, false)) - goto endjob; + goto cleanup; /* allow the migration job to be cancelled or the domain to be paused */ qemuDomainObjSetAsyncJobMask(vm, QEMU_DEFAULT_JOB_MASK | @@ -13218,24 +13198,24 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Invalid snapshot image format specified " "in configuration file")); - goto endjob; + goto cleanup; } if (!qemuCompressProgramAvailable(compressed)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Compression program for image format " "in configuration file isn't available")); - goto endjob; + goto cleanup; } } if (!(xml = qemuDomainDefFormatLive(driver, vm->def, true, true))) - goto endjob; + goto cleanup; if ((ret = qemuDomainSaveMemory(driver, vm, snap->def->file, xml, compressed, resume, 0, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) - goto endjob; + goto cleanup; /* the memory image was created, remove it on errors */ memory_unlink = true; @@ -13253,30 +13233,22 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, */ if ((ret = qemuDomainSnapshotCreateDiskActive(driver, vm, snap, flags, QEMU_ASYNC_JOB_SNAPSHOT)) < 0) - goto endjob; + goto cleanup; /* the snapshot is complete now */ if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) { - virObjectEventPtr event; - event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT, 0); virDomainAuditStop(vm, "from-snapshot"); - /* We already filtered the _HALT flag for persistent domains - * only, so this end job never drops the last reference. */ - ignore_value(qemuDomainObjEndAsyncJob(driver, vm)); resume = false; thaw = 0; - vm = NULL; if (event) qemuDomainEventQueue(driver, event); } else if (memory && pmsuspended) { /* qemu 1.3 is unable to save a domain in pm-suspended (S3) * state; so we must emit an event stating that it was * converted to paused. */ - virObjectEventPtr event; - virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, @@ -13287,12 +13259,11 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, ret = 0; - endjob: - if (resume && vm && virDomainObjIsActive(vm) && + cleanup: + if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, QEMU_ASYNC_JOB_SNAPSHOT) < 0) { - virObjectEventPtr event = NULL; event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); @@ -13306,21 +13277,14 @@ qemuDomainSnapshotCreateActiveExternal(virConnectPtr conn, ret = -1; goto cleanup; } - if (vm && thaw != 0 && + + if (thaw != 0 && qemuDomainSnapshotFSThaw(driver, vm, thaw > 0) < 0) { /* helper reported the error, if it was needed */ if (thaw > 0) ret = -1; } - if (vm && !qemuDomainObjEndAsyncJob(driver, vm)) { - /* Only possible if a transient vm quit while our locks were down, - * in which case we don't want to save snapshot metadata. - */ - *vmptr = NULL; - ret = -1; - } - cleanup: VIR_FREE(xml); virObjectUnref(cfg); if (memory_unlink && ret < 0) @@ -13466,10 +13430,19 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; } + /* We are going to modify the domain below. Internal snapshots would use + * a regular job, so we need to set the job mask to disallow query as + * 'savevm' blocks the monitor. External snapshot will then modify the + * job mas appropriately. */ + if (qemuDomainObjBeginAsyncJob(driver, vm, QEMU_ASYNC_JOB_SNAPSHOT) < 0) + goto cleanup; + + qemuDomainObjSetAsyncJobMask(vm, QEMU_JOB_NONE); + if (redefine) { if (virDomainSnapshotRedefinePrep(domain, vm, &def, &snap, &update_current, flags) < 0) - goto cleanup; + goto endjob; } else { /* Easiest way to clone inactive portion of vm->def is via * conversion in and back out of xml. */ @@ -13477,7 +13450,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, !(def->dom = virDomainDefParseString(xml, caps, driver->xmlopt, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; @@ -13499,7 +13472,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("internal snapshot of a running VM " "must include the memory state")); - goto cleanup; + goto endjob; } def->memory = (def->state == VIR_DOMAIN_SHUTOFF ? @@ -13509,12 +13482,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (virDomainSnapshotAlignDisks(def, align_location, align_match) < 0 || qemuDomainSnapshotPrepare(conn, vm, def, &flags) < 0) - goto cleanup; + goto endjob; } if (!snap) { if (!(snap = virDomainSnapshotAssignDef(vm->snapshots, def))) - goto cleanup; + goto endjob; def = NULL; } @@ -13524,12 +13497,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (vm->current_snapshot) { if (!redefine && VIR_STRDUP(snap->def->parent, vm->current_snapshot->def->name) < 0) - goto cleanup; + goto endjob; if (update_current) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, cfg->snapshotDir) < 0) - goto cleanup; + goto endjob; vm->current_snapshot = NULL; } } @@ -13544,13 +13517,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, snap->def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { /* external checkpoint or disk snapshot */ if (qemuDomainSnapshotCreateActiveExternal(domain->conn, driver, - &vm, snap, flags) < 0) - goto cleanup; + vm, snap, flags) < 0) + goto endjob; } else { /* internal checkpoint */ if (qemuDomainSnapshotCreateActiveInternal(domain->conn, driver, - &vm, snap, flags) < 0) - goto cleanup; + vm, snap, flags) < 0) + goto endjob; } } else { /* inactive; qemuDomainSnapshotPrepare guaranteed that we @@ -13561,10 +13534,10 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (qemuDomainSnapshotCreateInactiveExternal(driver, vm, snap, reuse) < 0) - goto cleanup; + goto endjob; } else { if (qemuDomainSnapshotCreateInactiveInternal(driver, vm, snap) < 0) - goto cleanup; + goto endjob; } } @@ -13574,34 +13547,41 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, */ snapshot = virGetDomainSnapshot(domain, snap->def->name); - cleanup: - if (vm) { - if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { - if (qemuDomainSnapshotWriteMetadata(vm, snap, - cfg->snapshotDir) < 0) { - /* if writing of metadata fails, error out rather than trying - * to silently carry on without completing the snapshot */ - virDomainSnapshotFree(snapshot); - snapshot = NULL; - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unable to save metadata for snapshot %s"), - snap->def->name); - virDomainSnapshotObjListRemove(vm->snapshots, snap); - } else { - if (update_current) - vm->current_snapshot = snap; - other = virDomainSnapshotFindByName(vm->snapshots, - snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; - } - } else if (snap) { + endjob: + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + cfg->snapshotDir) < 0) { + /* if writing of metadata fails, error out rather than trying + * to silently carry on without completing the snapshot */ + virDomainSnapshotFree(snapshot); + snapshot = NULL; + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unable to save metadata for snapshot %s"), + snap->def->name); virDomainSnapshotObjListRemove(vm->snapshots, snap); + } else { + if (update_current) + vm->current_snapshot = snap; + other = virDomainSnapshotFindByName(vm->snapshots, + snap->def->parent); + snap->parent = other; + other->nchildren++; + snap->sibling = other->first_child; + other->first_child = snap; } - virObjectUnlock(vm); + } else if (snap) { + virDomainSnapshotObjListRemove(vm->snapshots, snap); } + + if (!qemuDomainObjEndAsyncJob(driver, vm)) { + /* Only possible if a transient vm quit while our locks were down, + * in which case we don't want to save snapshot metadata. */ + vm = NULL; + } + + cleanup: + if (vm) + virObjectUnlock(vm); virDomainSnapshotDefFree(def); VIR_FREE(xml); virObjectUnref(caps); -- 2.0.2
On 09/03/2014 07:53 AM, Peter Krempa wrote:
Creating snapshots modifies the domain state. Currently we wouldn't enter the job for certain operations although they would modify the state. Refactor job handling so that everything is covered by an async job. --- src/qemu/qemu_driver.c | 176 ++++++++++++++++++++++--------------------------- 1 file changed, 78 insertions(+), 98 deletions(-)
@@ -13466,10 +13430,19 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, goto cleanup; }
+ /* We are going to modify the domain below. Internal snapshots would use + * a regular job, so we need to set the job mask to disallow query as + * 'savevm' blocks the monitor. External snapshot will then modify the + * job mas appropriately. */
s/mas/mask/
+ endjob: + if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (qemuDomainSnapshotWriteMetadata(vm, snap, + cfg->snapshotDir) < 0) { + /* if writing of metadata fails, error out rather than trying + * to silently carry on without completing the snapshot */
While reindenting this, you could also fix the accidental double space that I stuck in there.
+ + if (!qemuDomainObjEndAsyncJob(driver, vm)) { + /* Only possible if a transient vm quit while our locks were down, + * in which case we don't want to save snapshot metadata. */
This comment is now out of date, because you placed it below the point where you already saved the metadata. But overall, looks like the right thing to do - you widened the overall job to cover everything, rather than grabbing the job inside helper functions. ACK with the nits cleaned up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Jincheng Miao <jmiao@redhat.com> The code would lookup the snapshot object before acquiring the job. This could lead to a crash as one thread could delete the snapshot object, while a second thread already had the reference. Signed-off-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9f200f0..8e5c353 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14013,9 +14013,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + if (!vm->persistent && snap->def->state != VIR_DOMAIN_RUNNING && snap->def->state != VIR_DOMAIN_PAUSED && @@ -14024,13 +14027,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("transient domain needs to request run or pause " "to revert to inactive snapshot")); - goto cleanup; + goto endjob; } if (virDomainSnapshotIsExternal(snap)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("revert to external snapshot not supported yet")); - goto cleanup; + goto endjob; } if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_FORCE)) { @@ -14038,7 +14041,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, _("snapshot '%s' lacks domain '%s' rollback info"), snap->def->name, vm->def->name); - goto cleanup; + goto endjob; } if (virDomainObjIsActive(vm) && !(snap->def->state == VIR_DOMAIN_RUNNING @@ -14047,7 +14050,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s", _("must respawn qemu to start inactive snapshot")); - goto cleanup; + goto endjob; } } @@ -14056,7 +14059,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, cfg->snapshotDir) < 0) - goto cleanup; + goto endjob; vm->current_snapshot = NULL; /* XXX Should we restore vm->current_snapshot after this point * in the failure cases where we know there was no change? */ @@ -14071,12 +14074,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, driver->xmlopt, true); if (!config) - goto cleanup; + goto endjob; } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - switch ((virDomainState) snap->def->state) { case VIR_DOMAIN_RUNNING: case VIR_DOMAIN_PAUSED: @@ -14386,9 +14386,12 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (virDomainSnapshotDeleteEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; - if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot))) + goto endjob; + if (!metadata_only) { if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && virDomainSnapshotIsExternal(snap)) @@ -14401,13 +14404,10 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("deletion of %d external disk snapshots not " "supported yet"), external); - goto cleanup; + goto endjob; } } - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - if (flags & (VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY)) { rem.driver = driver; -- 2.0.2
On 09/03/2014 07:53 AM, Peter Krempa wrote:
From: Jincheng Miao <jmiao@redhat.com>
The code would lookup the snapshot object before acquiring the job. This could lead to a crash as one thread could delete the snapshot object, while a second thread already had the reference.
Signed-off-by: Jincheng Miao <jmiao@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8e5c353..9b2d355 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13589,9 +13589,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, return snapshot; } -static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, - int nameslen, - unsigned int flags) +static int +qemuDomainSnapshotListNames(virDomainPtr domain, + char **names, + int nameslen, + unsigned int flags) { virDomainObjPtr vm = NULL; int n = -1; @@ -13614,8 +13616,9 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, return n; } -static int qemuDomainSnapshotNum(virDomainPtr domain, - unsigned int flags) +static int +qemuDomainSnapshotNum(virDomainPtr domain, + unsigned int flags) { virDomainObjPtr vm = NULL; int n = -1; @@ -13638,7 +13641,8 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, } static int -qemuDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, +qemuDomainListAllSnapshots(virDomainPtr domain, + virDomainSnapshotPtr **snaps, unsigned int flags) { virDomainObjPtr vm = NULL; @@ -13750,9 +13754,10 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, return n; } -static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, - const char *name, - unsigned int flags) +static virDomainSnapshotPtr +qemuDomainSnapshotLookupByName(virDomainPtr domain, + const char *name, + unsigned int flags) { virDomainObjPtr vm; virDomainSnapshotObjPtr snap = NULL; @@ -13777,8 +13782,9 @@ static virDomainSnapshotPtr qemuDomainSnapshotLookupByName(virDomainPtr domain, return snapshot; } -static int qemuDomainHasCurrentSnapshot(virDomainPtr domain, - unsigned int flags) +static int +qemuDomainHasCurrentSnapshot(virDomainPtr domain, + unsigned int flags) { virDomainObjPtr vm; int ret = -1; @@ -13833,8 +13839,9 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, return parent; } -static virDomainSnapshotPtr qemuDomainSnapshotCurrent(virDomainPtr domain, - unsigned int flags) +static virDomainSnapshotPtr +qemuDomainSnapshotCurrent(virDomainPtr domain, + unsigned int flags) { virDomainObjPtr vm; virDomainSnapshotPtr snapshot = NULL; @@ -13861,8 +13868,9 @@ static virDomainSnapshotPtr qemuDomainSnapshotCurrent(virDomainPtr domain, return snapshot; } -static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, - unsigned int flags) +static char * +qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, + unsigned int flags) { virDomainObjPtr vm = NULL; char *xml = NULL; -- 2.0.2
On 09/03/2014 07:53 AM, Peter Krempa wrote:
--- src/qemu/qemu_driver.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
@@ -13614,8 +13616,9 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, return n; }
-static int qemuDomainSnapshotNum(virDomainPtr domain, - unsigned int flags) +static int +qemuDomainSnapshotNum(virDomainPtr domain,
Might as well use two blank lines between functions while doing this. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Return failure right away when the domain object can't be looked up instead of jumping to cleanup. This allows to remove the condition before unlocking the domain object. --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9b2d355..838200d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13602,7 +13602,7 @@ qemuDomainSnapshotListNames(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return -1; if (virDomainSnapshotListNamesEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13611,8 +13611,7 @@ qemuDomainSnapshotListNames(virDomainPtr domain, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13627,7 +13626,7 @@ qemuDomainSnapshotNum(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return -1; if (virDomainSnapshotNumEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13635,8 +13634,7 @@ qemuDomainSnapshotNum(virDomainPtr domain, n = virDomainSnapshotObjListNum(vm->snapshots, NULL, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13652,7 +13650,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return -1; if (virDomainListAllSnapshotsEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13660,8 +13658,7 @@ qemuDomainListAllSnapshots(virDomainPtr domain, n = virDomainListSnapshots(vm->snapshots, NULL, domain, snaps, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13679,7 +13676,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (virDomainSnapshotListChildrenNamesEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13691,8 +13688,7 @@ qemuDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13708,7 +13704,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (virDomainSnapshotNumChildrenEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13719,8 +13715,7 @@ qemuDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, n = virDomainSnapshotObjListNum(vm->snapshots, snap, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13737,7 +13732,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (virDomainSnapshotListAllChildrenEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13749,8 +13744,7 @@ qemuDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, flags); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return n; } @@ -13766,7 +13760,7 @@ qemuDomainSnapshotLookupByName(virDomainPtr domain, virCheckFlags(0, NULL); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return NULL; if (virDomainSnapshotLookupByNameEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13777,8 +13771,7 @@ qemuDomainSnapshotLookupByName(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, snap->def->name); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return snapshot; } @@ -13792,7 +13785,7 @@ qemuDomainHasCurrentSnapshot(virDomainPtr domain, virCheckFlags(0, -1); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return -1; if (virDomainHasCurrentSnapshotEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13800,8 +13793,7 @@ qemuDomainHasCurrentSnapshot(virDomainPtr domain, ret = (vm->current_snapshot != NULL); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return ret; } @@ -13816,7 +13808,7 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, virCheckFlags(0, NULL); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return NULL; if (virDomainSnapshotGetParentEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13834,8 +13826,7 @@ qemuDomainSnapshotGetParent(virDomainSnapshotPtr snapshot, parent = virGetDomainSnapshot(snapshot->domain, snap->def->parent); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return parent; } @@ -13849,7 +13840,7 @@ qemuDomainSnapshotCurrent(virDomainPtr domain, virCheckFlags(0, NULL); if (!(vm = qemuDomObjFromDomain(domain))) - goto cleanup; + return NULL; if (virDomainSnapshotCurrentEnsureACL(domain->conn, vm->def) < 0) goto cleanup; @@ -13863,8 +13854,7 @@ qemuDomainSnapshotCurrent(virDomainPtr domain, snapshot = virGetDomainSnapshot(domain, vm->current_snapshot->def->name); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return snapshot; } @@ -13880,7 +13870,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return NULL; if (virDomainSnapshotGetXMLDescEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13893,8 +13883,7 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, xml = virDomainSnapshotDefFormat(uuidstr, snap->def, flags, 0); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return xml; } @@ -13909,7 +13898,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (virDomainSnapshotIsCurrentEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13921,8 +13910,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot, STREQ(snapshot->name, vm->current_snapshot->def->name)); cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return ret; } @@ -13938,7 +13926,7 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, virCheckFlags(0, -1); if (!(vm = qemuDomObjFromSnapshot(snapshot))) - goto cleanup; + return -1; if (virDomainSnapshotHasMetadataEnsureACL(snapshot->domain->conn, vm->def) < 0) goto cleanup; @@ -13952,8 +13940,7 @@ qemuDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot, ret = 1; cleanup: - if (vm) - virObjectUnlock(vm); + virObjectUnlock(vm); return ret; } -- 2.0.2
On 09/03/2014 07:53 AM, Peter Krempa wrote:
Return failure right away when the domain object can't be looked up instead of jumping to cleanup. This allows to remove the condition before unlocking the domain object. --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 39 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/06/14 21:52, Eric Blake wrote:
On 09/03/2014 07:53 AM, Peter Krempa wrote:
Return failure right away when the domain object can't be looked up instead of jumping to cleanup. This allows to remove the condition before unlocking the domain object. --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 39 deletions(-)
ACK
Thanks. I've included stuff you requested in the review and pushed this series. Peter
participants (2)
-
Eric Blake -
Peter Krempa