When reverting a running domain to a paused snapshot, the event that fires should mention that the domain is suspended. * include/libvirt/libvirt.h.in (VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT): New sub-event. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use it. --- include/libvirt/libvirt.h.in | 3 ++- src/qemu/qemu_driver.c | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index aa29fb6..a625479 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2010,7 +2010,7 @@ typedef enum { VIR_DOMAIN_EVENT_STARTED_BOOTED = 0, /* Normal startup from boot */ VIR_DOMAIN_EVENT_STARTED_MIGRATED = 1, /* Incoming migration from another host */ VIR_DOMAIN_EVENT_STARTED_RESTORED = 2, /* Restored from a state file */ - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from snapshot */ + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT = 3, /* Restored from running snapshot */ } virDomainEventStartedDetailType; /** @@ -2023,6 +2023,7 @@ typedef enum { VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED = 1, /* Suspended for offline migration */ VIR_DOMAIN_EVENT_SUSPENDED_IOERROR = 2, /* Suspended due to a disk I/O error */ VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG = 3, /* Suspended due to a watchdog firing */ + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 4, /* Restored from paused snapshot */ } virDomainEventSuspendedDetailType; /** diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 421a98e..78d99f4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8804,14 +8804,16 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); } else { virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } - - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, -- 1.7.4.4

If you take a checkpoint snapshot of a running domain, then pause qemu, then restore the snapshot, the result should be a running domain, but the code was leaving things paused. Furthermore, if you take a checkpoint of a paused domain, then run, then restore, there was a brief but non-deterministic window of time where the domain was running rather than paused. Fix both of these discrepancies by always pausing before restoring. Also, check that the VM is active every time lock is dropped between two monitor calls. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Always pause before reversion. --- src/qemu/qemu_driver.c | 61 +++++++++++++++++++++++++++++++++-------------- 1 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 78d99f4..aa53c63 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8772,44 +8772,69 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->state == VIR_DOMAIN_RUNNING || snap->def->state == VIR_DOMAIN_PAUSED) { - + /* When using the loadvm monitor command, qemu does not know + * whether to pause or run the reverted domain, and just stays + * in the same state as before the monitor command, whether + * that is paused or running. We always pause before loadvm, + * to have finer control. */ if (virDomainObjIsActive(vm)) { priv = vm->privateData; + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + if (qemuProcessStopCPUs(driver, vm, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + /* Create an event now in case the restore fails, so + * that user will be alerted that they are now paused. + * If restore later succeeds to a running state, we + * replace this event with another. */ + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + } qemuDomainObjEnterMonitorWithDriver(driver, vm); rc = qemuMonitorLoadSnapshot(priv->mon, snap->def->name); qemuDomainObjExitMonitorWithDriver(driver, vm); - if (rc < 0) + if (rc < 0) { + /* XXX resume domain if it was running before the + * failed loadvm attempt? */ goto endjob; + } } else { if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) goto endjob; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - false, false, -1, NULL, VIR_VM_OP_CREATE); + true, false, -1, NULL, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); + if (rc < 0) + goto endjob; if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) goto endjob; - if (rc < 0) - goto endjob; } + /* Touch up domain state. */ if (snap->def->state == VIR_DOMAIN_PAUSED) { - /* qemu unconditionally starts the domain running again after - * loadvm, so let's pause it to keep consistency - * XXX we should have used qemuProcessStart's start_paused instead - */ - rc = qemuProcessStopCPUs(driver, vm, - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT, - QEMU_ASYNC_JOB_NONE); + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + } else { + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto endjob; + } + rc = qemuProcessStartCPUs(driver, vm, snapshot->domain->conn, + VIR_DOMAIN_RUNNING_FROM_SNAPSHOT, + QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT); - } else { - virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, - VIR_DOMAIN_RUNNING_FROM_SNAPSHOT); + virDomainEventFree(event); event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); -- 1.7.4.4

qemuDomainSnapshotRevertInactive has the same FIXMEs as qemuDomainSnapshotCreateInactive, so algorithmic fixes to properly handle partial loop iterations should be applied later to both functions, but we're not making the situation any worse in this patch. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use qemu-img rather than 'qemu -loadvm' to revert to offline snapshot. (qemuDomainSnapshotRevertInactive): New helper. --- src/qemu/qemu_driver.c | 65 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa53c63..989d21b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8734,6 +8734,52 @@ cleanup: return xml; } +/* The domain is expected to be locked and inactive. */ +static int +qemuDomainSnapshotRevertInactive(virDomainObjPtr vm, + virDomainSnapshotObjPtr snap) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-a", NULL, NULL, NULL }; + int ret = -1; + int i; + + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) { + /* qemuFindQemuImgBinary set the error */ + goto cleanup; + } + + qemuimgarg[3] = snap->def->name; + + for (i = 0; i < vm->def->ndisks; i++) { + /* FIXME: we also need to handle LVM here */ + /* FIXME: if we fail halfway through this loop, we are in an + * inconsistent state. I'm not quite sure what to do about that + */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Disk device '%s' does not support" + " snapshotting"), + vm->def->disks[i]->info.alias); + goto cleanup; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) + goto cleanup; + } + } + + ret = 0; + +cleanup: + VIR_FREE(qemuimgarg[0]); + return ret; +} + static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -8840,14 +8886,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); } } else { - /* qemu is a little funny with running guests and the restoration - * of snapshots. If the snapshot was taken online, - * then after a "loadvm" monitor command, the VM is set running - * again. If the snapshot was taken offline, then after a "loadvm" - * monitor command the VM is left paused. Unpausing it leads to - * the memory state *before* the loadvm with the disk *after* the - * loadvm, which obviously is bound to corrupt something. - * Therefore we destroy the domain and set it to "off" in this case. + /* Newer qemu -loadvm refuses to revert to the state of a snapshot + * created by qemu-img snapshot -c. If the domain is running, we + * must take it offline; then do the revert using qemu-img. */ if (virDomainObjIsActive(vm)) { @@ -8857,6 +8898,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); if (!vm->persistent) { + /* XXX it should be impossible to create an offline snapshot + * of a transient domain. Once we fix 'undefine' to convert + * a defined domain back to transient, that transition should + * be rejected if any offline snapshots exist. For now, we + * just stop the transient domain and quit, without reverting + * any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; @@ -8864,7 +8911,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } - if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) goto endjob; } -- 1.7.4.4

Changing the current vm, and writing that change to the file system, all before a new qemu starts, is risky; it's hard to roll back if starting the new qemu fails for some reason. Instead of abusing vm->current_snapshot and making the command line generator decide whether the current snapshot warrants using -loadvm, it is better to just directly pass a snapshot all the way through the call chain if it is to be loaded. This frees up the last use of snapshot->def->active for qemu's use, so the next patch can repurpose that field for tracking which snapshot is current. * src/qemu/qemu_command.c (qemuBuildCommandLine): Don't use active field of snapshot. * src/qemu/qemu_process.c (qemuProcessStart): Add a parameter. * src/qemu/qemu_process.h (qemuProcessStart): Update prototype. * src/qemu/qemu_migration.c (qemuMigrationPrepareAny): Update callers. * src/qemu/qemu_driver.c (qemudDomainCreate) (qemuDomainSaveImageStartVM, qemuDomainObjStart) (qemuDomainRevertToSnapshot): Likewise. (qemuDomainSnapshotSetCurrentActive) (qemuDomainSnapshotSetCurrentInactive): Delete unused functions. --- src/qemu/qemu_command.c | 7 ++--- src/qemu/qemu_driver.c | 47 ++++---------------------------------------- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_process.c | 10 +------- src/qemu/qemu_process.h | 1 + 5 files changed, 12 insertions(+), 55 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index dbfc7d9..81e0525 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2866,7 +2866,7 @@ qemuBuildCommandLine(virConnectPtr conn, virBitmapPtr qemuCaps, const char *migrateFrom, int migrateFd, - virDomainSnapshotObjPtr current_snapshot, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop) { int i; @@ -4782,9 +4782,8 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if (current_snapshot && current_snapshot->def->active) - virCommandAddArgList(cmd, "-loadvm", current_snapshot->def->name, - NULL); + if (snapshot) + virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); if (def->namespaceData) { qemuDomainCmdlineDefPtr qemucmd; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b83e1f0..7426690 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -804,12 +804,6 @@ qemudShutdown(void) { } -static int qemuDomainSnapshotSetCurrentActive(virDomainObjPtr vm, - char *snapshotDir); -static int qemuDomainSnapshotSetCurrentInactive(virDomainObjPtr vm, - char *snapshotDir); - - static virDrvOpenStatus qemudOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) @@ -1297,7 +1291,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuProcessStart(conn, driver, vm, NULL, (flags & VIR_DOMAIN_START_PAUSED) != 0, (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, - -1, NULL, VIR_VM_OP_CREATE) < 0) { + -1, NULL, NULL, VIR_VM_OP_CREATE) < 0) { virDomainAuditStart(vm, "booted", false); if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, @@ -3920,7 +3914,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, /* Set the migration source and start it up. */ ret = qemuProcessStart(conn, driver, vm, "stdio", true, - false, *fd, path, VIR_VM_OP_RESTORE); + false, *fd, path, NULL, VIR_VM_OP_RESTORE); if (intermediatefd != -1) { if (ret < 0) { @@ -4462,7 +4456,7 @@ qemuDomainObjStart(virConnectPtr conn, } ret = qemuProcessStart(conn, driver, vm, NULL, start_paused, - autodestroy, -1, NULL, VIR_VM_OP_CREATE); + autodestroy, -1, NULL, NULL, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "booted", ret >= 0); if (ret >= 0) { virDomainEventPtr event = @@ -8314,32 +8308,6 @@ cleanup: return ret; } -static int qemuDomainSnapshotSetCurrentActive(virDomainObjPtr vm, - char *snapshotDir) -{ - if (vm->current_snapshot) { - vm->current_snapshot->def->active = 1; - - return qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, - snapshotDir); - } - - return 0; -} - -static int qemuDomainSnapshotSetCurrentInactive(virDomainObjPtr vm, - char *snapshotDir) -{ - if (vm->current_snapshot) { - vm->current_snapshot->def->active = 0; - - return qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, - snapshotDir); - } - - return 0; -} - static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { @@ -8853,17 +8821,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } } else { - if (qemuDomainSnapshotSetCurrentActive(vm, driver->snapshotDir) < 0) - goto endjob; - rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - true, false, -1, NULL, VIR_VM_OP_CREATE); + true, false, -1, NULL, vm->current_snapshot, + VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) goto endjob; - if (qemuDomainSnapshotSetCurrentInactive(vm, - driver->snapshotDir) < 0) - goto endjob; } /* Touch up domain state. */ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 5510493..92d8a36 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1041,7 +1041,7 @@ qemuMigrationPrepareAny(struct qemud_driver *driver, * -incoming $migrateFrom */ if (qemuProcessStart(dconn, driver, vm, migrateFrom, true, - true, dataFD[0], NULL, + true, dataFD[0], NULL, NULL, VIR_VM_OP_MIGRATE_IN_START) < 0) { virDomainAuditStart(vm, "migrated", false); /* Note that we don't set an error here because qemuProcessStart diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6f54b30..27214ad 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2614,6 +2614,7 @@ int qemuProcessStart(virConnectPtr conn, bool autodestroy, int stdin_fd, const char *stdin_path, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop) { int ret; @@ -2816,16 +2817,9 @@ int qemuProcessStart(virConnectPtr conn, VIR_DEBUG("Building emulator command line"); if (!(cmd = qemuBuildCommandLine(conn, driver, vm->def, priv->monConfig, priv->monJSON != 0, priv->qemuCaps, - migrateFrom, stdin_fd, - vm->current_snapshot, vmop))) + migrateFrom, stdin_fd, snapshot, vmop))) goto cleanup; -#if 0 - /* XXX */ - if (qemuDomainSnapshotSetCurrentInactive(vm, driver->snapshotDir) < 0) - goto cleanup; -#endif - /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_QEMU)) { char *xml = virDomainDefFormat(vm->def, 0); diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h index e9b910d..96ba3f3 100644 --- a/src/qemu/qemu_process.h +++ b/src/qemu/qemu_process.h @@ -52,6 +52,7 @@ int qemuProcessStart(virConnectPtr conn, bool autodestroy, int stdin_fd, const char *stdin_path, + virDomainSnapshotObjPtr snapshot, enum virVMOperationType vmop); void qemuProcessStop(struct qemud_driver *driver, -- 1.7.4.4

A future patch will make it impossible to remove a domain if it would leave behind any libvirt-tracked metadata about snapshots, since stale metadata interferes with a new domain by the same name. But requiring snaphot contents to be deleted before removing a domain is harsh; with qemu, qemu-img can still make use of the contents after the libvirt domain is gone. Therefore, we need an option to get rid of libvirt tracking information, but not the actual contents. For hypervisors that do not track any metadata in libvirt, the implementation is trivial; all remaining hypervisors (really, just qemu) will be dealt with separately. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY): New flag. * src/libvirt.c (virDomainSnapshotDelete): Document it. * src/esx/esx_driver.c (esxDomainSnapshotDelete): Trivially supported when there is no libvirt metadata. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotDelete): Likewise. --- include/libvirt/libvirt.h.in | 3 ++- src/esx/esx_driver.c | 10 +++++++++- src/libvirt.c | 10 +++++++--- src/vbox/vbox_tmpl.c | 10 +++++++++- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index a625479..eae0a10 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2579,7 +2579,8 @@ int virDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, /* Delete a snapshot */ typedef enum { - VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = (1 << 0), + VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN = (1 << 0), /* Also delete children */ + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY = (1 << 1), /* Delete just metadata */ } virDomainSnapshotDeleteFlags; int virDomainSnapshotDelete(virDomainSnapshotPtr snapshot, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index c097651..beeafbd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4543,7 +4543,8 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) esxVI_TaskInfoState taskInfoState; char *taskInfoErrorMessage = NULL; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); if (esxVI_EnsureSession(priv->primary) < 0) { return -1; @@ -4561,6 +4562,13 @@ esxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) goto cleanup; } + /* ESX snapshots do not require any libvirt metadata, making this + * flag trivial once we know we have a valid snapshot. */ + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { + result = 0; + goto cleanup; + } + if (esxVI_RemoveSnapshot_Task(priv->primary, snapshotTree->snapshot, removeChildren, &task) < 0 || esxVI_WaitForTaskCompletion(priv->primary, task, snapshot->domain->uuid, diff --git a/src/libvirt.c b/src/libvirt.c index c8af3e1..8ee9e96 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15792,14 +15792,18 @@ error: /** * virDomainSnapshotDelete: * @snapshot: a domain snapshot object - * @flags: flag parameters + * @flags: bitwise-or of supported virDomainSnapshotDeleteFlags * * Delete the snapshot. * * If @flags is 0, then just this snapshot is deleted, and changes from * this snapshot are automatically merged into children snapshots. If - * flags is VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, then this snapshot - * and any children snapshots are deleted. + * @flags includes VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, then this snapshot + * and any children snapshots are deleted. If @flags includes + * VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, then any snapshot metadata + * tracked by libvirt is removed while keeping the snapshot contents + * intact; if a hypervisor does not require any libvirt metadata to + * track snapshots, then this flag is silently ignored. * * Returns 0 if the snapshot was successfully deleted, -1 on error. */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 822e899..8de2bae 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -6361,7 +6361,8 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, PRUint32 state; nsresult rc; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | + VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1); vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); @@ -6382,6 +6383,13 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } + /* VBOX snapshots do not require any libvirt metadata, making this + * flag trivial once we know we have a valid snapshot. */ + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) { + ret = 0; + goto cleanup; + } + if (state >= MachineState_FirstOnline && state <= MachineState_LastOnline) { vboxError(VIR_ERR_OPERATION_INVALID, "%s", -- 1.7.4.4

Similar to the last patch in isolating the filtering from the client actions, so that clients don't have to reinvent the filtering. * src/conf/domain_conf.h (virDomainSnapshotForEachChild): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainSnapshotActOnChild) (virDomainSnapshotForEachChild): New functions. (virDomainSnapshotCountChildren): Delete. (virDomainSnapshotHasChildren): Simplify. * src/qemu/qemu_driver.c (qemuDomainSnapshotReparentChildren) (qemuDomainSnapshotDelete): Likewise. --- src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++------------- src/conf/domain_conf.h | 4 +++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 31 ++++++++++++++--------------- 4 files changed, 54 insertions(+), 30 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7704849..f1b0aca 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11252,32 +11252,52 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virHashRemoveEntry(snapshots->objs, snapshot->def->name); } -struct snapshot_has_children { - char *name; +struct snapshot_act_on_child { + char *parent; int number; + virHashIterator iter; + void *data; }; -static void virDomainSnapshotCountChildren(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) +static void +virDomainSnapshotActOnChild(void *payload, + const void *name, + void *data) { virDomainSnapshotObjPtr obj = payload; - struct snapshot_has_children *curr = data; + struct snapshot_act_on_child *curr = data; - if (obj->def->parent && STREQ(obj->def->parent, curr->name)) + if (obj->def->parent && STREQ(curr->parent, obj->def->parent)) { curr->number++; + if (curr->iter) + (curr->iter)(payload, name, curr->data); + } } -int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, - virDomainSnapshotObjListPtr snapshots) +/* Run iter(data) on all direct children of snapshot, while ignoring all + * other entries in snapshots. Return the number of children + * visited. No particular ordering is guaranteed. */ +int +virDomainSnapshotForEachChild(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data) { - struct snapshot_has_children children; + struct snapshot_act_on_child act; - children.name = snap->def->name; - children.number = 0; - virHashForEach(snapshots->objs, virDomainSnapshotCountChildren, &children); + act.parent = snapshot->def->name; + act.number = 0; + act.iter = iter; + act.data = data; + virHashForEach(snapshots->objs, virDomainSnapshotActOnChild, &act); - return children.number; + return act.number; +} + +int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, + virDomainSnapshotObjListPtr snapshots) +{ + return virDomainSnapshotForEachChild(snapshots, snap, NULL, NULL); } typedef enum { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d266605..5f752ec 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1343,6 +1343,10 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, virDomainSnapshotObjListPtr snapshots); +int virDomainSnapshotForEachChild(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); int virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot, virHashIterator iter, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c53b295..0d8aa99 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -384,6 +384,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefParseString; virDomainSnapshotFindByName; +virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotHasChildren; virDomainSnapshotObjListGetNames; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f523404..9134fc3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9042,7 +9042,7 @@ qemuDomainSnapshotDiscardDescendant(void *payload, struct snap_reparent { struct qemud_driver *driver; - virDomainSnapshotObjPtr snap; + const char *parent; virDomainObjPtr vm; int err; }; @@ -9059,22 +9059,20 @@ qemuDomainSnapshotReparentChildren(void *payload, return; } - if (snap->def->parent && STREQ(snap->def->parent, rep->snap->def->name)) { - VIR_FREE(snap->def->parent); + VIR_FREE(snap->def->parent); - if (rep->snap->def->parent != NULL) { - snap->def->parent = strdup(rep->snap->def->parent); + if (rep->parent != NULL) { + snap->def->parent = strdup(rep->parent); - if (snap->def->parent == NULL) { - virReportOOMError(); - rep->err = -1; - return; - } + if (snap->def->parent == NULL) { + virReportOOMError(); + rep->err = -1; + return; } - - rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, - rep->driver->snapshotDir); } + + rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, + rep->driver->snapshotDir); } static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, @@ -9122,11 +9120,12 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto endjob; } else { rep.driver = driver; - rep.snap = snap; + rep.parent = snap->def->parent; rep.vm = vm; rep.err = 0; - virHashForEach(vm->snapshots.objs, qemuDomainSnapshotReparentChildren, - &rep); + virDomainSnapshotForEachChild(&vm->snapshots, snap, + qemuDomainSnapshotReparentChildren, + &rep); if (rep.err < 0) goto endjob; } -- 1.7.4.4

To make it easier to know when undefine will fail because of existing snapshot metadata, we need to know how many snapshots have metadata. Also, it is handy to filter the list of snapshots to just those that have no parents; document that flag now, but implement it in later patches. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_LIST_ROOTS) (VIR_DOMAIN_SNAPSHOT_LIST_METADATA): New flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotListNames): Document them. * src/esx/esx_driver.c (esxDomainSnapshotNum) (esxDomainSnapshotListNames): Implement trivial flag. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotNum) (vboxDomainSnapshotListNames): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotNum) (qemuDomainSnapshotListNames): Likewise. --- include/libvirt/libvirt.h.in | 9 +++++++++ src/esx/esx_driver.c | 10 +++++++--- src/libvirt.c | 27 ++++++++++++++++++++++----- src/qemu/qemu_driver.c | 8 ++++++-- src/vbox/vbox_tmpl.c | 15 +++++++++++++-- 5 files changed, 57 insertions(+), 12 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index eae0a10..20fdbdf 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2551,6 +2551,15 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags); +/* Flags valid for both virDomainSnapshotNum() and + * virDomainSnapshotListNames(). */ +typedef enum { + VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots which + have no parents */ + VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots which + have metadata */ +} virDomainSnapshotListFlags; + /* Return the number of snapshots for this domain */ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index beeafbd..dbc7694 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4329,12 +4329,16 @@ esxDomainSnapshotNum(virDomainPtr domain, unsigned int flags) esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); if (esxVI_EnsureSession(priv->primary) < 0) { return -1; } + /* ESX snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) + return 0; + if (esxVI_LookupRootSnapshotTreeList(priv->primary, domain->uuid, &rootSnapshotTreeList) < 0) { return -1; @@ -4357,14 +4361,14 @@ esxDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, esxPrivate *priv = domain->conn->privateData; esxVI_VirtualMachineSnapshotTree *rootSnapshotTreeList = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); if (names == NULL || nameslen < 0) { ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument")); return -1; } - if (nameslen == 0) { + if (nameslen == 0 || (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA)) { return 0; } diff --git a/src/libvirt.c b/src/libvirt.c index 8ee9e96..30b464a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15526,11 +15526,19 @@ error: /** * virDomainSnapshotNum: * @domain: a domain object - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-or of supported virDomainSnapshotListFlags + * + * Provides the number of domain snapshots for this domain. * - * Provides the number of domain snapshots for this domain.. + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is + * filtered to the number of snapshots that have no parents. * - * Returns the number of domain snapshost found or -1 in case of error. + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. + * + * Returns the number of domain snapshots found or -1 in case of error. */ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags) @@ -15566,11 +15574,20 @@ error: * @domain: a domain object * @names: array to collect the list of names of snapshots * @nameslen: size of @names - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-or of supported virDomainSnapshotListFlags * * Collect the list of domain snapshots for the given domain, and store * their names in @names. Caller is responsible for freeing each member - * of the array. + * of the array. The value to use for @nameslen can be determined by + * virDomainSnapshotNum() with the same @flags. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is + * filtered to the number of snapshots that have no parents. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is + * the number of snapshots that also include metadata that would prevent + * the removal of the last reference to a domain; this value will either + * be 0 or the same value as if the flag were not given. * * Returns the number of domain snapshots found or -1 in case of error. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 568cb37..3e6d653 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8536,7 +8536,7 @@ static int qemuDomainSnapshotListNames(virDomainPtr domain, char **names, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8564,7 +8564,7 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, virDomainObjPtr vm = NULL; int n = -1; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, domain->uuid); @@ -8576,6 +8576,10 @@ static int qemuDomainSnapshotNum(virDomainPtr domain, goto cleanup; } + /* All qemu snapshots have libvirt metadata, so + * VIR_DOMAIN_SNAPSHOT_LIST_METADATA makes no difference to our + * answer. */ + n = virDomainSnapshotObjListNum(&vm->snapshots); cleanup: diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 8de2bae..fc9739e 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5861,7 +5861,7 @@ vboxDomainSnapshotNum(virDomainPtr dom, nsresult rc; PRUint32 snapshotCount; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine); @@ -5871,6 +5871,12 @@ vboxDomainSnapshotNum(virDomainPtr dom, goto cleanup; } + /* VBox snapshots do not require libvirt to maintain any metadata. */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + ret = 0; + goto cleanup; + } + rc = machine->vtbl->GetSnapshotCount(machine, &snapshotCount); if (NS_FAILED(rc)) { vboxError(VIR_ERR_INTERNAL_ERROR, @@ -5901,7 +5907,7 @@ vboxDomainSnapshotListNames(virDomainPtr dom, int count = 0; int i; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine); @@ -5911,6 +5917,11 @@ vboxDomainSnapshotListNames(virDomainPtr dom, goto cleanup; } + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) { + ret = 0; + goto cleanup; + } + if ((count = vboxDomainSnapshotGetAll(dom, machine, &snapshots)) < 0) goto cleanup; -- 1.7.4.4

Prepare for code sharing. No semantic change. * src/qemu/qemu_driver.c (qemuFindQemuImgBinary) (qemuDomainSnapshotWriteMetadata) (qemuDomainSnapshotDiscard): Float up. (qemuDomainSnapshotDiscardDescendant): Likewise, and rename... (qemuDomainSnapshotDiscardAll): ...for generic use. (qemuDomainSnapshotDelete): Update caller. --- src/qemu/qemu_driver.c | 370 ++++++++++++++++++++++++------------------------ 1 files changed, 187 insertions(+), 183 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 754ab71..3977135 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1580,6 +1580,192 @@ cleanup: } +struct snap_remove { + struct qemud_driver *driver; + virDomainObjPtr vm; + bool metadata_only; + int err; +}; + +/* Locate an appropriate 'qemu-img' binary. */ +static char * +qemuFindQemuImgBinary(void) +{ + char *ret; + + ret = virFindFileInPath("kvm-img"); + if (ret == NULL) + ret = virFindFileInPath("qemu-img"); + if (ret == NULL) + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("unable to find kvm-img or qemu-img")); + + return ret; +} + +static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, + virDomainSnapshotObjPtr snapshot, + char *snapshotDir) +{ + int fd = -1; + char *newxml = NULL; + int ret = -1; + char *snapDir = NULL; + char *snapFile = NULL; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + + virUUIDFormat(vm->def->uuid, uuidstr); + newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); + if (newxml == NULL) { + virReportOOMError(); + return -1; + } + + if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + if (virFileMakePath(snapDir) < 0) { + virReportSystemError(errno, _("cannot create snapshot directory '%s'"), + snapDir); + goto cleanup; + } + + if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR); + if (fd < 0) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("failed to create snapshot file '%s'"), snapFile); + goto cleanup; + } + if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { + virReportSystemError(errno, _("Failed to write snapshot data to %s"), + snapFile); + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FREE(snapFile); + VIR_FREE(snapDir); + VIR_FREE(newxml); + VIR_FORCE_CLOSE(fd); + return ret; +} + +/* Discard one snapshot (or its metadata), without reparenting any children. */ +static int +qemuDomainSnapshotDiscard(struct qemud_driver *driver, + virDomainObjPtr vm, + virDomainSnapshotObjPtr snap, + bool metadata_only) +{ + const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; + char *snapFile = NULL; + int ret = -1; + int i; + qemuDomainObjPrivatePtr priv; + virDomainSnapshotObjPtr parentsnap; + + if (!metadata_only) { + if (!virDomainObjIsActive(vm)) { + qemuimgarg[0] = qemuFindQemuImgBinary(); + if (qemuimgarg[0] == NULL) + /* qemuFindQemuImgBinary set the error */ + goto cleanup; + + qemuimgarg[3] = snap->def->name; + + for (i = 0; i < vm->def->ndisks; i++) { + /* FIXME: we also need to handle LVM here */ + if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + /* we continue on even in the face of error, since other + * disks in this VM may have this snapshot in place + */ + continue; + } + + qemuimgarg[4] = vm->def->disks[i]->src; + + if (virRun(qemuimgarg, NULL) < 0) { + /* we continue on even in the face of error, since other + * disks in this VM may have this snapshot in place + */ + continue; + } + } + } + } else { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + /* we continue on even in the face of error */ + qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + } + + if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, + vm->def->name, snap->def->name) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (snap == vm->current_snapshot) { + if (snap->def->parent) { + parentsnap = virDomainSnapshotFindByName(&vm->snapshots, + snap->def->parent); + if (!parentsnap) { + VIR_WARN("missing parent snapshot matching name '%s'", + snap->def->parent); + } else { + parentsnap->def->current = true; + if (qemuDomainSnapshotWriteMetadata(vm, snap, + driver->snapshotDir) < 0) { + VIR_WARN("failed to set parent snapshot '%s' as current", + snap->def->parent); + parentsnap->def->current = false; + parentsnap = NULL; + } + } + } + vm->current_snapshot = parentsnap; + } + + if (unlink(snapFile) < 0) + VIR_WARN("Failed to unlink %s", snapFile); + virDomainSnapshotObjListRemove(&vm->snapshots, snap); + + ret = 0; + +cleanup: + VIR_FREE(snapFile); + VIR_FREE(qemuimgarg[0]); + + return ret; +} + +/* Hash iterator callback to discard multiple snapshots. */ +static void +qemuDomainSnapshotDiscardAll(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + struct snap_remove *curr = data; + int err; + + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, + curr->metadata_only); + if (err && !curr->err) + curr->err = err; +} + static int qemuDomainDestroyFlags(virDomainPtr dom, unsigned int flags) @@ -8278,75 +8464,6 @@ cleanup: return ret; } -static char *qemuFindQemuImgBinary(void) -{ - char *ret; - - ret = virFindFileInPath("kvm-img"); - if (ret == NULL) - ret = virFindFileInPath("qemu-img"); - if (ret == NULL) - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("unable to find kvm-img or qemu-img")); - - return ret; -} - -static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, - virDomainSnapshotObjPtr snapshot, - char *snapshotDir) -{ - int fd = -1; - char *newxml = NULL; - int ret = -1; - char *snapDir = NULL; - char *snapFile = NULL; - char uuidstr[VIR_UUID_STRING_BUFLEN]; - - virUUIDFormat(vm->def->uuid, uuidstr); - newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); - if (newxml == NULL) { - virReportOOMError(); - return -1; - } - - if (virAsprintf(&snapDir, "%s/%s", snapshotDir, vm->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - if (virFileMakePath(snapDir) < 0) { - virReportSystemError(errno, _("cannot create snapshot directory '%s'"), - snapDir); - goto cleanup; - } - - if (virAsprintf(&snapFile, "%s/%s.xml", snapDir, snapshot->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - fd = open(snapFile, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR); - if (fd < 0) { - qemuReportError(VIR_ERR_OPERATION_FAILED, - _("failed to create snapshot file '%s'"), snapFile); - goto cleanup; - } - if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { - virReportSystemError(errno, _("Failed to write snapshot data to %s"), - snapFile); - goto cleanup; - } - - ret = 0; - -cleanup: - VIR_FREE(snapFile); - VIR_FREE(snapDir); - VIR_FREE(newxml); - VIR_FORCE_CLOSE(fd); - return ret; -} - - static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm) { int i; @@ -8965,119 +9082,6 @@ cleanup: return ret; } -static int qemuDomainSnapshotDiscard(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap, - bool metadata_only) -{ - const char *qemuimgarg[] = { NULL, "snapshot", "-d", NULL, NULL, NULL }; - char *snapFile = NULL; - int ret = -1; - int i; - qemuDomainObjPrivatePtr priv; - virDomainSnapshotObjPtr parentsnap = NULL; - - if (!metadata_only) { - if (!virDomainObjIsActive(vm)) { - qemuimgarg[0] = qemuFindQemuImgBinary(); - if (qemuimgarg[0] == NULL) - /* qemuFindQemuImgBinary set the error */ - goto cleanup; - - qemuimgarg[3] = snap->def->name; - - for (i = 0; i < vm->def->ndisks; i++) { - /* FIXME: we also need to handle LVM here */ - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (!vm->def->disks[i]->driverType || - STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { - /* we continue on even in the face of error, since other - * disks in this VM may have this snapshot in place - */ - continue; - } - - qemuimgarg[4] = vm->def->disks[i]->src; - - if (virRun(qemuimgarg, NULL) < 0) { - /* we continue on even in the face of error, since other - * disks in this VM may have this snapshot in place - */ - continue; - } - } - } - } else { - priv = vm->privateData; - qemuDomainObjEnterMonitorWithDriver(driver, vm); - /* we continue on even in the face of error */ - qemuMonitorDeleteSnapshot(priv->mon, snap->def->name); - qemuDomainObjExitMonitorWithDriver(driver, vm); - } - } - - if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir, - vm->def->name, snap->def->name) < 0) { - virReportOOMError(); - goto cleanup; - } - - if (snap == vm->current_snapshot) { - if (snap->def->parent) { - parentsnap = virDomainSnapshotFindByName(&vm->snapshots, - snap->def->parent); - if (!parentsnap) { - VIR_WARN("missing parent snapshot matching name '%s'", - snap->def->parent); - } else { - parentsnap->def->current = true; - if (qemuDomainSnapshotWriteMetadata(vm, snap, - driver->snapshotDir) < 0) { - VIR_WARN("failed to set parent snapshot '%s' as current", - snap->def->parent); - parentsnap->def->current = false; - parentsnap = NULL; - } - } - } - vm->current_snapshot = parentsnap; - } - - if (unlink(snapFile) < 0) - VIR_WARN("Failed to unlink %s", snapFile); - virDomainSnapshotObjListRemove(&vm->snapshots, snap); - - ret = 0; - -cleanup: - VIR_FREE(snapFile); - VIR_FREE(qemuimgarg[0]); - - return ret; -} - -struct snap_remove { - struct qemud_driver *driver; - virDomainObjPtr vm; - bool metadata_only; - int err; -}; - -static void -qemuDomainSnapshotDiscardDescendant(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) -{ - virDomainSnapshotObjPtr snap = payload; - struct snap_remove *curr = data; - int err; - - err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, - curr->metadata_only); - if (err && !curr->err) - curr->err = err; -} - struct snap_reparent { struct qemud_driver *driver; const char *parent; @@ -9155,7 +9159,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rem.err = 0; virDomainSnapshotForEachDescendant(&vm->snapshots, snap, - qemuDomainSnapshotDiscardDescendant, + qemuDomainSnapshotDiscardAll, &rem); if (rem.err < 0) goto endjob; -- 1.7.4.4

A nice benefit of deleting all snapshots at undefine time is that you don't have to do any reparenting or subtree identification - since everything goes, this is an O(n) process whereas using multiple virDomainSnapshotDelete calls would be O(n^2) or worse. * src/qemu/qemu_driver.c (qemuDomainDestroyFlags) (qemuDomainUndefineFlags): Honor new flags. --- src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++++----------- 1 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f4a4786..027fdee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1488,7 +1488,7 @@ static int qemuDomainShutdown(virDomainPtr dom) { } if (!vm->persistent && - (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots))) { + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("cannot delete transient domain with %d snapshots"), nsnapshots); @@ -1777,7 +1777,8 @@ qemuDomainDestroyFlags(virDomainPtr dom, qemuDomainObjPrivatePtr priv; int nsnapshots; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1790,11 +1791,25 @@ qemuDomainDestroyFlags(virDomainPtr dom, } if (!vm->persistent && - (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots))) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("cannot delete transient domain with %d snapshots"), - nsnapshots); - goto cleanup; + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + struct snap_remove rem; + + if ((flags & (VIR_DOMAIN_DESTROY_SNAPSHOTS_METADATA | + VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete transient domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = !(flags & VIR_DOMAIN_DESTROY_SNAPSHOTS_FULL); + rem.err = 0; + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + if (rem.err < 0) + goto cleanup; } priv = vm->privateData; @@ -4918,7 +4933,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, int ret = -1; int nsnapshots; - virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); + virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4937,11 +4954,25 @@ qemuDomainUndefineFlags(virDomainPtr dom, qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot delete active domain")); goto cleanup; - } else if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots))) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("cannot delete inactive domain with %d snapshots"), - nsnapshots); - goto cleanup; + } else if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + struct snap_remove rem; + + if ((flags & (VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = !(flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_FULL); + rem.err = 0; + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + if (rem.err < 0) + goto cleanup; } if (!vm->persistent) { -- 1.7.4.4

Migration is another case of stranding metadata. And since snapshot metadata is arbitrarily large, there's no way to shoehorn it into the migration cookie of migration v3. A future patch will make it possible to manually recreate the snapshot metadata on the destination. But even that is limited, since if we delete the snapshot metadata prior to migration, then we won't know the name of the current snapshot to pass along; and if we delete the snapshot metadata after migration and use the v3 migration cookie to pass along the name of the current snapshot, then we need a way to bypass the fact that this patch refuses migration with snapshot metadata present. So eventually, we may have to introduce migration protocol v4 that allows feature negotiation and an arbitrary number of handshake exchanges, so as to pass as many rpc calls as needed to transfer all the snapshot xml hierarchy. But all of that is thoughts for the future; for now, the best course of action is to quit early, rather than get into a funky state of stale metadata. * src/qemu/qemu_driver.c (qemudDomainMigratePerform) (qemuDomainMigrateBegin3, qemuDomainMigratePerform3): Add restriction. --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 027fdee..70fe607 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7723,6 +7723,7 @@ qemudDomainMigratePerform (virDomainPtr dom, virDomainObjPtr vm; int ret = -1; const char *dconnuri = NULL; + int nsnapshots; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -7743,6 +7744,13 @@ qemudDomainMigratePerform (virDomainPtr dom, goto cleanup; } + if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + if (flags & VIR_MIGRATE_PEER2PEER) { dconnuri = uri; uri = NULL; @@ -7819,6 +7827,7 @@ qemuDomainMigrateBegin3(virDomainPtr domain, struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm; char *xml = NULL; + int nsnapshots; virCheckFlags(QEMU_MIGRATION_FLAGS, NULL); @@ -7832,6 +7841,13 @@ qemuDomainMigrateBegin3(virDomainPtr domain, goto cleanup; } + if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + if ((flags & VIR_MIGRATE_CHANGE_PROTECTION)) { if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup; @@ -7993,6 +8009,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + int nsnapshots; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -8006,6 +8023,13 @@ qemuDomainMigratePerform3(virDomainPtr dom, goto cleanup; } + if ((nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot migrate domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + ret = qemuMigrationPerform(driver, dom->conn, vm, xmlin, dconnuri, uri, cookiein, cookieinlen, cookieout, cookieoutlen, -- 1.7.4.4

Just like VM saved state images (virsh save), snapshots MUST track the inactive domain xml to detect any ABI incompatibilities. The indentation is not perfect, but functionality comes before form. Later patches will actually supply a full domain; for now, this wires up the storage to support one, but doesn't ever generate one in dumpxml output. Happily, libvirt.c was already rejecting use of VIR_DOMAIN_XML_SECURE from read-only connections, even though before this patch, there was no one ever using that flag and there was no information to be secured by the use of that flag. * src/libvirt.c (virDomainSnapshotGetXMLDesc): Document flag. * src/conf/domain_conf.h (_virDomainSnapshotDef): Add member. (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Update signature. * src/conf/domain_conf.c (virDomainSnapshotDefFree): Clean up. (virDomainSnapshotDefParseString): Optionally parse domain. (virDomainSnapshotDefFormat): Output full domain. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML) (esxDomainSnapshotGetXMLDesc): Update callers. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML) (vboxDomainSnapshotGetXMLDesc): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotLoad, qemuDomainSnapshotGetXMLDesc) (qemuDomainSnapshotWriteMetadata): Likewise. * docs/formatsnapshot.html.in: Rework doc example. Based on a patch by Philipp Hahn. --- docs/formatsnapshot.html.in | 45 ++++++++++++++++++++++++++------------ src/conf/domain_conf.c | 50 +++++++++++++++++++++++++++++++++++++------ src/conf/domain_conf.h | 7 +++++- src/esx/esx_driver.c | 4 +- src/libvirt.c | 7 +++++- src/qemu/qemu_driver.c | 27 ++++++++++++++++------- src/vbox/vbox_tmpl.c | 4 +- 7 files changed, 109 insertions(+), 35 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 79ed1d2..85dcc7f 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -46,27 +46,44 @@ snapshots. Readonly. </dd> <dt><code>domain</code></dt> - <dd>The domain that this snapshot was taken against. This - element contains exactly one child element, uuid. This - specifies the uuid of the domain that this snapshot was taken - against. Readonly. + <dd>The domain that this snapshot was taken against. Older + versions of libvirt stored only a single child element, uuid; + reverting to a snapshot like this is risky if the current state + of the domain differs from the state that the domain was created + in, and requires the use of the VIR_DOMAIN_SNAPSHOT_REVERT_FORCE + flag. Newer versions of libvirt store the entire + inactive <a href="formatdomain.html">domain configuration</a> at + the time of the snapshot. Readonly. </dd> </dl> - <h2><a name="example">Example</a></h2> + <h2><a name="example">Examples</a></h2> + <p>Using this XML on creation:</p> <pre> <domainsnapshot> - <name>os-updates</name> <description>Snapshot of OS install and updates</description> - <state>running</state> - <creationTime>1270477159</creationTime> - <parent> - <name>bare-os-install</name> - </parent> - <domain> - <uuid>93a5c045-6457-2c09-e56c-927cdf34e178</uuid> - </domain> </domainsnapshot></pre> + + <p>Will result in XML similar to this from + virDomainSnapshotGetXMLDesc:</p> + <pre> +<domainsnapshot> + <name>1270477159</name> + <description>Snapshot of OS install and updates</description> + <state>running</state> + <creationTime>1270477159</creationTime> + <parent> + <name>bare-os-install</name> + </parent> + <domain> + <name>fedora</name> + <uuid>93a5c045-6457-2c09-e56c-927cdf34e178</uuid> + <memory>1048576</memory> + ... + </devices> + </domain> +</domainsnapshot></pre> + </body> </html> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b586bc8..ade8a02 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10960,11 +10960,17 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + virDomainDefFree(def->dom); VIR_FREE(def); } -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) +/* If newSnapshot is true, caps, expectedVirtTypes, and flags are ignored. */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -10973,6 +10979,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, char *creation = NULL, *state = NULL; struct timeval tv; int active; + char *tmp; xml = virXMLParse(NULL, xmlStr, "domainsnapshot.xml"); if (!xml) { @@ -11042,9 +11049,29 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, goto cleanup; } def->current = active != 0; - } - else + + /* Older snapshots were created with just <domain>/<uuid>, and + * lack domain/@type. In that case, leave dom NULL, and + * clients will have to decide between best effort + * initialization or outright failure. */ + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + VIR_FREE(tmp); + xmlNodePtr domainNode = virXPathNode("./domain", ctxt); + if (!domainNode) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + def->dom = virDomainDefParseNode(caps, xml, domainNode, + expectedVirtTypes, flags); + if (!def->dom) + goto cleanup; + } else { + VIR_WARN("parsing older snapshot that lacks domain"); + } + } else { def->creationTime = tv.tv_sec; + } ret = def; @@ -11061,10 +11088,15 @@ cleanup: char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal) { virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + flags |= VIR_DOMAIN_XML_INACTIVE; + virBufferAddLit(&buf, "<domainsnapshot>\n"); virBufferAsprintf(&buf, " <name>%s</name>\n", def->name); if (def->description) @@ -11079,9 +11111,13 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); - virBufferAddLit(&buf, " <domain>\n"); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); - virBufferAddLit(&buf, " </domain>\n"); + if (def->dom) { + virDomainDefFormatInternal(def->dom, flags, &buf); + } else { + virBufferAddLit(&buf, " <domain>\n"); + virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", domain_uuid); + virBufferAddLit(&buf, " </domain>\n"); + } if (internal) virBufferAsprintf(&buf, " <active>%d</active>\n", def->current); virBufferAddLit(&buf, "</domainsnapshot>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 503fb58..93bb7c8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1302,6 +1302,7 @@ struct _virDomainSnapshotDef { char *parent; long long creationTime; /* in seconds */ int state; + virDomainDefPtr dom; /* Internal use. */ bool current; /* At most one snapshot in the list should have this set */ @@ -1325,10 +1326,14 @@ struct _virDomainSnapshotObjList { }; virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + int newSnapshot, + virCapsPtr caps, + unsigned int expectedVirtTypes, + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, + unsigned int flags, int internal); virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 90f55c3..8fbbe8a 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4223,7 +4223,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0); if (def == NULL) { return NULL; @@ -4319,7 +4319,7 @@ esxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virUUIDFormat(snapshot->domain->uuid, uuid_string); - xml = virDomainSnapshotDefFormat(uuid_string, &def, 0); + xml = virDomainSnapshotDefFormat(uuid_string, &def, flags, 0); cleanup: esxVI_VirtualMachineSnapshotTree_Free(&rootSnapshotList); diff --git a/src/libvirt.c b/src/libvirt.c index 61b3548..9911433 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15511,10 +15511,15 @@ error: /** * virDomainSnapshotGetXMLDesc: * @snapshot: a domain snapshot object - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-OR of subset of virDomainXMLFlags * * Provide an XML description of the domain snapshot. * + * No security-sensitive data will be included unless @flags contains + * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only + * connections. For this API, @flags should not contain either + * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 70fe607..13224ab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -337,7 +337,10 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, 0, qemu_driver->caps, + QEMU_EXPECTED_VIRT_TYPES, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -1604,9 +1607,10 @@ qemuFindQemuImgBinary(struct qemud_driver *driver) return driver->qemuImgBinary; } -static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, - virDomainSnapshotObjPtr snapshot, - char *snapshotDir) +static int +qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, + virDomainSnapshotObjPtr snapshot, + char *snapshotDir) { int fd = -1; char *newxml = NULL; @@ -1616,7 +1620,8 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, char uuidstr[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(vm->def->uuid, uuidstr); - newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, 1); + newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, + VIR_DOMAIN_XML_SECURE, 1); if (newxml == NULL) { virReportOOMError(); return -1; @@ -1642,6 +1647,12 @@ static int qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, _("failed to create snapshot file '%s'"), snapFile); goto cleanup; } + /* XXX need virsh snapshot-edit, before this makes sense: + * char *tmp; + * virAsprintf(&tmp, "snapshot-edit %s", vm->def->name); + * virEmitXMLWarning(fd, snapshot->def->name, tmp); + * VIR_FREE(tmp); + */ if (safewrite(fd, newxml, strlen(newxml)) != strlen(newxml)) { virReportSystemError(errno, _("Failed to write snapshot data to %s"), snapFile); @@ -8671,7 +8682,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) @@ -8898,7 +8909,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; - virCheckFlags(0, NULL); + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); @@ -8917,7 +8928,7 @@ static char *qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, goto cleanup; } - xml = virDomainSnapshotDefFormat(uuidstr, snap->def, 0); + xml = virDomainSnapshotDefFormat(uuidstr, snap->def, flags, 0); cleanup: if (vm) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 9e1c6e3..0dd5efa 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5661,7 +5661,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, virCheckFlags(0, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); @@ -5843,7 +5843,7 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, def->state = VIR_DOMAIN_SHUTOFF; virUUIDFormat(dom->uuid, uuidstr); - ret = virDomainSnapshotDefFormat(uuidstr, def, 0); + ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0); cleanup: virDomainSnapshotDefFree(def); -- 1.7.4.4

When reverting to a snapshot, the inactive domain configuration has to be rolled back to what it was at the time of the snapshot. Additionally, if the VM is active and the snapshot was active, this now adds a failure if the two configurations are ABI incompatible, rather than risking qemu confusion. A future patch will add a VIR_DOMAIN_SNAPSHOT_FORCE flag, which will be required for three risky code paths - reverting to an older snapshot that lacked full domain information, reverting from running to a live snapshot that requires starting a new qemu process, and any reverting that stops a running vm. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Copy out domain. (qemuDomainRevertToSnapshot): Perform ABI compatibility checks. --- src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 51 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 13224ab..a8ea73d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8656,6 +8656,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, { struct qemud_driver *driver = domain->conn->privateData; virDomainObjPtr vm = NULL; + char *xml = NULL; virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -8703,6 +8704,15 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, vm->current_snapshot = NULL; } + /* Easiest way to clone inactive portion of vm->def is via + * conversion in and back out of xml. */ + if (!(xml = virDomainDefFormat(vm->def, (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE))) || + !(def->dom = virDomainDefParseString(driver->caps, xml, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; + /* actually do the snapshot */ if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) @@ -8733,6 +8743,7 @@ cleanup: virDomainSnapshotDefFree(def); virDomainObjUnlock(vm); } + VIR_FREE(xml); qemuDriverUnlock(driver); return snapshot; } @@ -8994,6 +9005,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainEventPtr event = NULL; qemuDomainObjPrivatePtr priv; int rc; + virDomainDefPtr config = NULL; virCheckFlags(0, -1); @@ -9024,7 +9036,30 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * in the failure cases where we know there was no change? */ } + /* Prepare to copy the snapshot inactive xml as the config of this + * domain. Easiest way is by a round trip through xml. + * + * XXX Should domain snapshots track live xml rather + * than inactive xml? */ snap->def->current = true; + if (snap->def->dom) { + char *xml; + if (!(xml = virDomainDefFormat(snap->def->dom, + (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)))) + goto cleanup; + config = virDomainDefParseString(driver->caps, xml, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE); + VIR_FREE(xml); + if (!config) + goto cleanup; + } else { + /* XXX Fail if VIR_DOMAIN_REVERT_FORCE is not set, rather than + * blindly hoping for the best. */ + VIR_WARN("snapshot is lacking rollback information for domain '%s'", + snap->def->name); + } if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; @@ -9037,6 +9072,15 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * that is paused or running. We always pause before loadvm, * to have finer control. */ if (virDomainObjIsActive(vm)) { + + /* Check for ABI compatibility. */ + if (config && !virDomainDefCheckABIStability(vm->def, config)) { + /* XXX Add VIR_DOMAIN_REVERT_FORCE to permit killing + * and restarting a new qemu, since loadvm monitor + * command won't work. */ + goto endjob; + } + priv = vm->privateData; if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { if (qemuProcessStopCPUs(driver, vm, @@ -9064,7 +9108,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * failed loadvm attempt? */ goto endjob; } + if (config) + virDomainObjAssignDef(vm, config, false); } else { + if (config) + virDomainObjAssignDef(vm, config, false); + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, true, false, -1, NULL, vm->current_snapshot, VIR_VM_OP_CREATE); @@ -9121,6 +9170,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (qemuDomainSnapshotRevertInactive(driver, vm, snap) < 0) goto endjob; + if (config) + virDomainObjAssignDef(vm, config, false); } ret = 0; -- 1.7.4.4

As discussed here: https://www.redhat.com/archives/libvir-list/2011-August/msg00361.html https://www.redhat.com/archives/libvir-list/2011-August/msg00552.html Adds: <devices> <disk type=... snapshot='no|internal|external'> ... <transient/> </disk> </devices> * docs/schemas/domaincommon.rng (snapshot): New define. (disk): Add snapshot and persistent attributes. * docs/formatdomain.html.in: Document them. * src/conf/domain_conf.h (virDomainDiskSnapshot): New enum. (_virDomainDiskDef): New fields. --- docs/formatdomain.html.in | 40 ++++++++++++++++++++++++++++++++++++---- docs/schemas/domaincommon.rng | 17 +++++++++++++++++ src/conf/domain_conf.c | 35 +++++++++++++++++++++++++++++++++-- src/conf/domain_conf.h | 12 ++++++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 100 insertions(+), 6 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f46771d..911dee5 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -889,7 +889,7 @@ <pre> ... <devices> - <disk type='file'> + <disk type='file' snapshot='external'> <driver name="tap" type="aio" cache="default"/> <source file='/var/lib/xen/images/fv0'/> <target dev='hda' bus='ide'/> @@ -910,8 +910,14 @@ </source> <target dev="hdb" bus="ide"/> <boot order='1'/> + <transient/> <address type='drive' controller='0' bus='1' unit='0'/> </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <target def='hdc' bus='ide'/> + <readonly/> + </disk> </devices> ...</pre> @@ -923,9 +929,23 @@ and refers to the underlying source for the disk. The optional <code>device</code> attribute indicates how the disk is to be exposed to the guest OS. Possible values for this attribute are "floppy", "disk" - and "cdrom", defaulting to "disk". - <span class="since">Since 0.0.3; "device" attribute since 0.1.4; - "network" attribute since 0.8.7</span></dd> + and "cdrom", defaulting to "disk". The + optional <code>snapshot</code> attribute indicates the default + behavior of the disk during disk snapshots: "internal" + requires a file format such as qcow2 that can store both the + snapshot and the data changes since the snapshot; + "external" will separate the snapshot from the live data; and + "no" means the disk will not participate in snapshots. + Read-only disks default to "no", while the default for other + disks depends on the hypervisor's capabilities. Some + hypervisors allow a per-snapshot choice as well, + during <a href="formatsnapshot.html">domain snapshot + creation</a>. Not all snapshot modes are supported; + for example, <code>snapshot='yes'</code> with a transient disk + generally does not make sense. <span class="since">Since 0.0.3; + "device" attribute since 0.1.4; + "network" attribute since 0.8.7; "snapshot" since + 0.9.5</span></dd> <dt><code>source</code></dt> <dd>If the disk <code>type</code> is "file", then the <code>file</code> attribute specifies the fully-qualified @@ -1032,11 +1052,23 @@ the <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> + <dt><code>readonly</code></dt> + <dd>If present, this indicates the device cannot be modified by + the guest. For now, this is the default for disks with + attribute <code>type='cdrom'</code>. + </dd> <dt><code>shareable</code></dt> <dd>If present, this indicates the device is expected to be shared between domains (assuming the hypervisor and OS support this), which means that caching should be deactivated for that device. </dd> + <dt><code>transient</code></dt> + <dd>If present, this indicates that changes to the device + contents should be reverted automatically when the guest + exits. With some hypervisors, marking a disk transient + prevents the domain from participating in migration or + snapshots. <span class="since">Since 0.9.5</span> + </dd> <dt><code>serial</code></dt> <dd>If present, this specify serial number of virtual hard drive. For example, it may look diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 756e892..0af9e0f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -617,6 +617,11 @@ </element> </optional> <optional> + <element name="transient"> + <empty/> + </element> + </optional> + <optional> <element name="serial"> <ref name="diskSerial"/> </element> @@ -628,6 +633,15 @@ <ref name="address"/> </optional> </define> + <define name="snapshot"> + <attribute name="snapshot"> + <choice> + <value>no</value> + <value>internal</value> + <value>external</value> + </choice> + </attribute> + </define> <define name="lease"> <element name="lease"> @@ -667,6 +681,9 @@ </choice> </attribute> </optional> + <optional> + <ref name="snapshot"/> + </optional> <choice> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ade8a02..d2800d2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -187,6 +187,12 @@ VIR_ENUM_IMPL(virDomainVirtioEventIdx, VIR_DOMAIN_VIRTIO_EVENT_IDX_LAST, "on", "off") +VIR_ENUM_IMPL(virDomainDiskSnapshot, VIR_DOMAIN_DISK_SNAPSHOT_LAST, + "default", + "no", + "internal", + "external") + VIR_ENUM_IMPL(virDomainController, VIR_DOMAIN_CONTROLLER_TYPE_LAST, "ide", "fdc", @@ -2073,6 +2079,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr cur, host; char *type = NULL; char *device = NULL; + char *snapshot = NULL; char *driverName = NULL; char *driverType = NULL; char *source = NULL; @@ -2106,6 +2113,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->type = VIR_DOMAIN_DISK_TYPE_FILE; } + snapshot = virXMLPropString(node, "snapshot"); + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -2207,6 +2216,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { def->shared = 1; + } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) { + def->transient = 1; } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ @@ -2278,6 +2289,18 @@ virDomainDiskDefParseXML(virCapsPtr caps, goto error; } + if (snapshot) { + def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + if (def->snapshot <= 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot setting '%s'"), + snapshot); + goto error; + } + } else if (def->readonly) { + def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_NO; + } + if (bus) { if ((def->bus = virDomainDiskBusTypeFromString(bus)) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -2423,6 +2446,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, cleanup: VIR_FREE(bus); VIR_FREE(type); + VIR_FREE(snapshot); VIR_FREE(target); VIR_FREE(source); while (nhosts > 0) { @@ -2448,7 +2472,7 @@ cleanup: no_memory: virReportOOMError(); - error: +error: virDomainDiskDefFree(def); def = NULL; goto cleanup; @@ -8643,8 +8667,13 @@ virDomainDiskDefFormat(virBufferPtr buf, } virBufferAsprintf(buf, - " <disk type='%s' device='%s'>\n", + " <disk type='%s' device='%s'", type, device); + if (def->snapshot && + !(def->snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO && def->readonly)) + virBufferAsprintf(buf, " snapshot='%s'", + virDomainDiskSnapshotTypeToString(def->snapshot)); + virBufferAddLit(buf, ">\n"); if (def->driverName || def->driverType || def->cachemode || def->ioeventfd || def->event_idx) { @@ -8718,6 +8747,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, " <readonly/>\n"); if (def->shared) virBufferAddLit(buf, " <shareable/>\n"); + if (def->transient) + virBufferAddLit(buf, " <transient/>\n"); if (def->serial) virBufferEscapeString(buf, " <serial>%s</serial>\n", def->serial); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 93bb7c8..7dbf353 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -217,6 +217,15 @@ enum virDomainVirtioEventIdx { VIR_DOMAIN_VIRTIO_EVENT_IDX_LAST }; +enum virDomainDiskSnapshot { + VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT = 0, + VIR_DOMAIN_DISK_SNAPSHOT_NO, + VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL, + VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + + VIR_DOMAIN_DISK_SNAPSHOT_LAST +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -238,8 +247,10 @@ struct _virDomainDiskDef { int iomode; int ioeventfd; int event_idx; + int snapshot; /* enum virDomainDiskSnapshot */ unsigned int readonly : 1; unsigned int shared : 1; + unsigned int transient : 1; virDomainDeviceInfo info; virStorageEncryptionPtr encryption; }; @@ -1689,6 +1700,7 @@ VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainDiskErrorPolicy) VIR_ENUM_DECL(virDomainDiskProtocol) VIR_ENUM_DECL(virDomainDiskIo) +VIR_ENUM_DECL(virDomainDiskSnapshot) VIR_ENUM_DECL(virDomainIoEventFd) VIR_ENUM_DECL(virDomainVirtioEventIdx) VIR_ENUM_DECL(virDomainController) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0d8aa99..6e313a9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -286,6 +286,8 @@ virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; virDomainDiskRemove; virDomainDiskRemoveByName; +virDomainDiskSnapshotTypeFromString; +virDomainDiskSnapshotTypeToString; virDomainDiskTypeFromString; virDomainDiskTypeToString; virDomainFSDefFree; -- 1.7.4.4

The previous patch introduced new config, but if a hypervisor does not support that new config, someone can write XML that does not behave as documented. This prevents some of those cases by explicitly rejecting transient disks for several hypervisors. Disk snapshots will require a new flag to actually affect a snapshot creation, so there's not much to reject there. * src/qemu/qemu_command.c (qemuBuildDriveStr): Reject transient disks for now. * src/libxl/libxl_conf.c (libxlMakeDisk): Likewise. * src/xenxs/xen_sxpr.c (xenFormatSxprDisk): Likewise. * src/xenxs/xen_xm.c (xenFormatXMDisk): Likewise. --- src/libxl/libxl_conf.c | 5 +++++ src/qemu/qemu_command.c | 5 +++++ src/xenxs/xen_sxpr.c | 5 +++++ src/xenxs/xen_xm.c | 5 +++++ 4 files changed, 20 insertions(+), 0 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 09f3be8..b9bce14 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -537,6 +537,11 @@ libxlMakeDisk(virDomainDefPtr def, virDomainDiskDefPtr l_disk, x_disk->unpluggable = 1; x_disk->readwrite = !l_disk->readonly; x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0; + if (l_disk->transient) { + libxlError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libxenlight does not support transient disks")); + return -1; + } x_disk->domid = def->id; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 81e0525..35975f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1498,6 +1498,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (disk->readonly && qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) virBufferAddLit(&opt, ",readonly=on"); + if (disk->transient) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("transient disks not supported yet")); + goto error; + } if (disk->driverType && *disk->driverType != '\0' && disk->type != VIR_DOMAIN_DISK_TYPE_DIR && qemuCapsGet(qemuCaps, QEMU_CAPS_DRIVE_FORMAT)) diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 1f5be5f..5f87ce9 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1710,6 +1710,11 @@ xenFormatSxprDisk(virDomainDiskDefPtr def, virBufferAddLit(buf, "(mode 'w!')"); else virBufferAddLit(buf, "(mode 'w')"); + if (def->transient) { + XENXS_ERROR(VIR_ERR_CONFIG_UNSUPPORTED, + _("transient disks not supported yet")); + return -1; + } if (!isAttach) virBufferAddLit(buf, ")"); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index cb31226..03857c8 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1164,6 +1164,11 @@ static int xenFormatXMDisk(virConfValuePtr list, virBufferAddLit(&buf, ",!"); else virBufferAddLit(&buf, ",w"); + if (disk->transient) { + XENXS_ERROR(VIR_ERR_CONFIG_UNSUPPORTED, + _("transient disks not supported yet")); + return -1; + } if (virBufferError(&buf)) { virReportOOMError(); -- 1.7.4.4

New flag bits are worth exposing via virsh. Additionally, even though I recently added 'virsh snapshot-parent', doing it one snapshot at a time is painful, so make it possible to expand the snapshot-list table at once. In the case of snapshot-list --roots, it's possible to emulate this even when talking to an older server that lacks the bit; whereas --metadata requires a newer server. * tools/virsh.c (cmdSnapshotDumpXML, cmdSnapshotCurrent): Add --security-info. (cmdSnapshotList): Add --parent, --roots, --metadata. * tools/virsh.pod (snapshot-dumpxml, snapshot-current) (snapshot-list): Document these. --- Adding this also helped me find a couple of tweaks to make earlier in the series, where I've replied to those issues separately. tools/virsh.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 22 +++++++++++--- 2 files changed, 90 insertions(+), 13 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index bb08d4c..f5049c6 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12337,6 +12337,8 @@ static const vshCmdInfo info_snapshot_current[] = { static const vshCmdOptDef opts_snapshot_current[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"name", VSH_OT_BOOL, 0, N_("list the name, rather than the full xml")}, + {"security-info", VSH_OT_BOOL, 0, + N_("include security sensitive information in XML dump")}, {NULL, 0, 0, NULL} }; @@ -12348,6 +12350,10 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) int current; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12365,7 +12371,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (!(snapshot = virDomainSnapshotCurrent(dom, 0))) goto cleanup; - xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + xml = virDomainSnapshotGetXMLDesc(snapshot, flags); if (!xml) goto cleanup; @@ -12418,6 +12424,10 @@ static const vshCmdInfo info_snapshot_list[] = { static const vshCmdOptDef opts_snapshot_list[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"parent", VSH_OT_BOOL, 0, N_("add a column showing parent snapshot")}, + {"roots", VSH_OT_BOOL, 0, N_("list only snapshots without parents")}, + {"metadata", VSH_OT_BOOL, 0, + N_("list only snapshots that have metadata that would prevent undefine")}, {NULL, 0, 0, NULL} }; @@ -12426,6 +12436,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; + int flags = 0; + int parent_filter = 0; /* -1 for roots filtering, 0 for no parent + information needed, 1 for parent column */ int numsnaps; char **names = NULL; int actual = 0; @@ -12435,11 +12448,27 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char *doc = NULL; virDomainSnapshotPtr snapshot = NULL; char *state = NULL; + char *parent = NULL; long long creation_longlong; time_t creation_time_t; char timestr[100]; struct tm time_info; + if (vshCommandOptBool(cmd, "parent")) { + if (vshCommandOptBool(cmd, "roots")) { + vshError(ctl, "%s", + _("--parent and --roots are mutually exlusive")); + return false; + } + parent_filter = 1; + } else if (vshCommandOptBool(cmd, "roots")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + } + + if (vshCommandOptBool(cmd, "metadata")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; + } + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12447,19 +12476,35 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - numsnaps = virDomainSnapshotNum(dom, 0); + numsnaps = virDomainSnapshotNum(dom, flags); + + /* Fall back to simulation if --roots was unsupported. */ + if (numsnaps < 0 && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + virFreeError(last_error); + last_error = NULL; + parent_filter = -1; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + numsnaps = virDomainSnapshotNum(dom, flags); + } if (numsnaps < 0) goto cleanup; - vshPrintExtra(ctl, " %-20s %-25s %s\n", _("Name"), _("Creation Time"), _("State")); - vshPrintExtra(ctl, "---------------------------------------------------\n"); + if (parent_filter > 0) + vshPrintExtra(ctl, " %-20s %-25s %-15s %s", + _("Name"), _("Creation Time"), _("State"), _("Parent")); + else + vshPrintExtra(ctl, " %-20s %-25s %s", + _("Name"), _("Creation Time"), _("State")); + vshPrintExtra(ctl, "\n\ +------------------------------------------------------------\n"); if (numsnaps) { if (VIR_ALLOC_N(names, numsnaps) < 0) goto cleanup; - actual = virDomainSnapshotListNames(dom, names, numsnaps, 0); + actual = virDomainSnapshotListNames(dom, names, numsnaps, flags); if (actual < 0) goto cleanup; @@ -12467,6 +12512,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < actual; i++) { /* free up memory from previous iterations of the loop */ + VIR_FREE(parent); VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); @@ -12492,6 +12538,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!ctxt) continue; + if (parent_filter) { + parent = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); + if (!parent && parent_filter < 0) + continue; + } + state = virXPathString("string(/domainsnapshot/state)", ctxt); if (state == NULL) continue; @@ -12504,9 +12557,14 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) continue; } localtime_r(&creation_time_t, &time_info); - strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", &time_info); + strftime(timestr, sizeof(timestr), "%Y-%m-%d %H:%M:%S %z", + &time_info); - vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); + if (parent) + vshPrint(ctl, " %-20s %-25s %-15s %s\n", + names[i], timestr, state, parent); + else + vshPrint(ctl, " %-20s %-25s %s\n", names[i], timestr, state); } } @@ -12514,6 +12572,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) cleanup: /* this frees up memory from the last iteration of the loop */ + VIR_FREE(parent); VIR_FREE(state); if (snapshot) virDomainSnapshotFree(snapshot); @@ -12542,6 +12601,8 @@ static const vshCmdInfo info_snapshot_dumpxml[] = { static const vshCmdOptDef opts_snapshot_dumpxml[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"security-info", VSH_OT_BOOL, 0, + N_("include security sensitive information in XML dump")}, {NULL, 0, 0, NULL} }; @@ -12553,6 +12614,10 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + int flags = 0; + + if (vshCommandOptBool(cmd, "security-info")) + flags |= VIR_DOMAIN_XML_SECURE; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12568,7 +12633,7 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd) if (snapshot == NULL) goto cleanup; - xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + xml = virDomainSnapshotGetXMLDesc(snapshot, flags); if (!xml) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index c27e99d..64c3895 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -523,7 +523,7 @@ Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be used. I<--inactive> tells virsh to dump domain configuration that will be used on next start of the domain as opposed to the current domain configuration. -Using I<--security-info> security sensitive information will also be included +Using I<--security-info> will also include security sensitive information in the XML dump. I<--update-cpu> updates domain CPU requirements according to host CPU. @@ -1597,19 +1597,31 @@ Create a snapshot for domain I<domain> with the given <name> and value. If I<--print-xml> is specified, then XML appropriate for I<snapshot-create> is output, rather than actually creating a snapshot. -=item B<snapshot-current> I<domain> [I<--name>] +=item B<snapshot-current> I<domain> [I<--name>] [I<--security-info>] Output the snapshot XML for the domain's current snapshot (if any). If I<--name> is specified, just list the snapshot name instead of the -full xml. +full xml. Otherwise, using I<--security-info> will also include security +sensitive information. -=item B<snapshot-list> I<domain> +=item B<snapshot-list> I<domain> [{I<--parent> | I<--roots>}] [I<--metadata>] List all of the available snapshots for the given domain. -=item B<snapshot-dumpxml> I<domain> I<snapshot> +If I<--parent> is specified, add a column to the output table giving +the name of the parent of each snapshot. + +If I<--roots> is specified, the list will be filtered to just snapshots +that have no parents; this option is not compatible with I<--parent>. + +If I<--metadata> is specified, the list will be filtered to just +snapshots that involve libvirt metadata, and thus would interfere in +using the B<undefine> or B<destroy> commands on that domain. + +=item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] Output the snapshot XML for the domain's snapshot named I<snapshot>. +Using I<--security-info> will also include security sensitive information. =item B<snapshot-parent> I<domain> I<snapshot> -- 1.7.4.4

It would technically be possible to have virsh compute the list of descendants of a given snapshot, then delete those one at a time. But it's complex, and not worth writing for a first cut at implementing the new flags. * tools/virsh.c (cmdSnapshotDelete): Add --children-only, --metadata. * tools/virsh.pod (snapshot-delete): Document them. --- tools/virsh.c | 15 ++++++++++++++- tools/virsh.pod | 12 ++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index f5049c6..275a289 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12791,6 +12791,9 @@ static const vshCmdOptDef opts_snapshot_delete[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, {"children", VSH_OT_BOOL, 0, N_("delete snapshot and all children")}, + {"children-only", VSH_OT_BOOL, 0, N_("delete children but not snapshot")}, + {"metadata", VSH_OT_BOOL, 0, + N_("delete only libvirt metadata, leaving snapshot contents behind")}, {NULL, 0, 0, NULL} }; @@ -12815,13 +12818,23 @@ cmdSnapshotDelete(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "children")) flags |= VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN; + if (vshCommandOptBool(cmd, "children-only")) + flags |= VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY; + if (vshCommandOptBool(cmd, "metadata")) + flags |= VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY; snapshot = virDomainSnapshotLookupByName(dom, name, 0); if (snapshot == NULL) goto cleanup; + /* XXX If we wanted, we could emulate DELETE_CHILDREN_ONLY even on + * older servers that reject the flag, by manually computing the + * list of descendants. But that's a lot of code to maintain. */ if (virDomainSnapshotDelete(snapshot, flags) == 0) { - vshPrint(ctl, _("Domain snapshot %s deleted\n"), name); + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) + vshPrint(ctl, _("Domain snapshot %s children deleted\n"), name); + else + vshPrint(ctl, _("Domain snapshot %s deleted\n"), name); } else { vshError(ctl, _("Failed to delete snapshot %s"), name); goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 64c3895..4005242 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1635,12 +1635,20 @@ snapshot was taken will be lost. Also note that the state of the domain after snapshot-revert is complete will be the state of the domain at the time the original snapshot was taken. -=item B<snapshot-delete> I<domain> I<snapshot> I<--children> +=item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>] +[{I<--children> | I<--children-only>}] Delete the snapshot for the domain named I<snapshot>. If this snapshot has child snapshots, changes from this snapshot will be merged into the children. If I<--children> is passed, then delete this snapshot and any -children of this snapshot. +children of this snapshot. If I<--children-only> is passed, then delete +any children of this snapshot, but leave this snapshot intact. These +two flags are mutually exclusive. + +If I<--metadata> is specified, then only delete the snapshot metadata +maintained by libvirt, while leaving the snapshot contents intact for +access by external tools; otherwise deleting a snapshot also removes +the data contents from that point in time. =back -- 1.7.4.4

The next patch will make snapshot creation more complex, so it's better to avoid repetition of the complexity. * tools/virsh.c (vshSnapshotCreate): New helper function. (cmdSnapshotCreate, cmdSnapshotCreateAs): Use it. --- This patch requires a prerequisite series: https://www.redhat.com/archives/libvir-list/2011-August/msg00817.html tools/virsh.c | 119 ++++++++++++++++++++++++--------------------------------- 1 files changed, 50 insertions(+), 69 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index c803eea..e2f08ca 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12082,6 +12082,54 @@ cmdQuit(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) return true; } +/* Helper for snapshot-create and snapshot-create-as */ +static bool +vshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, + unsigned int flags, const char *from) +{ + bool ret = false; + virDomainSnapshotPtr snapshot; + char *doc = NULL; + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; + char *name = NULL; + + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + if (snapshot == NULL) + goto cleanup; + + doc = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (!doc) + goto cleanup; + + xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); + if (!xml) + goto cleanup; + + name = virXPathString("string(/domainsnapshot/name)", ctxt); + if (!name) { + vshError(ctl, "%s", + _("Could not find 'name' element in domain snapshot XML")); + goto cleanup; + } + + if (from) + vshPrint(ctl, _("Domain snapshot %s created from '%s'"), name, from); + else + vshPrint(ctl, _("Domain snapshot %s created"), name); + + ret = true; + +cleanup: + VIR_FREE(name); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + if (snapshot) + virDomainSnapshotFree(snapshot); + VIR_FREE(doc); + return ret; +} + /* * "snapshot-create" command */ @@ -12104,11 +12152,6 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *from = NULL; char *buffer = NULL; - virDomainSnapshotPtr snapshot = NULL; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; - char *doc = NULL; - char *name = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12134,39 +12177,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - snapshot = virDomainSnapshotCreateXML(dom, buffer, 0); - if (snapshot == NULL) - goto cleanup; - - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) - goto cleanup; - - xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); - if (!xml) - goto cleanup; - - name = virXPathString("string(/domainsnapshot/name)", ctxt); - if (!name) { - vshError(ctl, "%s", - _("Could not find 'name' element in domain snapshot XML")); - goto cleanup; - } - - vshPrint(ctl, _("Domain snapshot %s created"), name); - if (from) - vshPrint(ctl, _(" from '%s'"), from); - vshPrint(ctl, "\n"); - - ret = true; + ret = vshSnapshotCreate(ctl, dom, buffer, 0, from); cleanup: - VIR_FREE(name); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - if (snapshot) - virDomainSnapshotFree(snapshot); - VIR_FREE(doc); VIR_FREE(buffer); if (dom) virDomainFree(dom); @@ -12197,13 +12210,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; bool ret = false; char *buffer = NULL; - virDomainSnapshotPtr snapshot = NULL; - xmlDocPtr xml = NULL; - xmlXPathContextPtr ctxt = NULL; - char *doc = NULL; const char *name = NULL; const char *desc = NULL; - char *parsed_name = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -12238,36 +12246,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - snapshot = virDomainSnapshotCreateXML(dom, buffer, 0); - if (snapshot == NULL) - goto cleanup; - - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); - if (!doc) - goto cleanup; - - xml = virXMLParseStringCtxt(doc, "domainsnapshot.xml", &ctxt); - if (!xml) - goto cleanup; - - parsed_name = virXPathString("string(/domainsnapshot/name)", ctxt); - if (!parsed_name) { - vshError(ctl, "%s", - _("Could not find 'name' element in domain snapshot XML")); - goto cleanup; - } - - vshPrint(ctl, _("Domain snapshot %s created\n"), name ? name : parsed_name); - - ret = true; + ret = vshSnapshotCreate(ctl, dom, buffer, 0, NULL); cleanup: - VIR_FREE(parsed_name); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(xml); - if (snapshot) - virDomainSnapshotFree(snapshot); - VIR_FREE(doc); VIR_FREE(buffer); if (dom) virDomainFree(dom); -- 1.7.4.4

In order to distinguish disk snapshots from system checkpoints, a new state value that is only valid for snapshots is helpful. * include/libvirt/libvirt.h.in (VIR_DOMAIN_LAST): New placeholder. * src/conf/domain_conf.h (virDomainSnapshotState): New enum mapping. * src/conf/domain_conf.c (virDomainState): Use placeholder. (virDomainSnapshotState): Extend mapping by one for use in snapshot. (virDomainSnapshotDefParseString, virDomainSnapshotDefFormat): Handle new state. (virDomainObjSetState, virDomainStateReasonToString) (virDomainStateReasonFromString): Avoid compiler warnings. * tools/virsh.c (vshDomainState, vshDomainStateReasonToString): Likewise. (cmdList): Translate state output. * src/libvirt_private.syms (domain_conf.h): Export new functions. * docs/schemas/domainsnapshot.rng: Tighten state definition. * docs/formatsnapshot.html.in: Document it. * tests/domainsnapshotxml2xmlout/disk_snapshot.xml: New test. --- docs/formatsnapshot.html.in | 82 +++++++++++++++++++--- docs/schemas/domainsnapshot.rng | 15 ++++- include/libvirt/libvirt.h.in | 14 ++++- src/conf/domain_conf.c | 26 +++++-- src/conf/domain_conf.h | 3 +- src/libvirt_private.syms | 2 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 35 +++++++++ tools/virsh.c | 10 ++- 8 files changed, 165 insertions(+), 22 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index edf20e8..21c83a0 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -7,6 +7,61 @@ <h2><a name="SnapshotAttributes">Snapshot XML</a></h2> <p> + There are several types of snapshots: + </p> + <dl> + <dt>disk snapshot</dt> + <dd>Contents of disks (whether a subset or all disks associated + with the domain) are saved at a given point of time, and can + be restored back to that state. On a running guest, a disk + snapshot is likely to be only crash-consistent rather than + clean (that is, it represents the state of the disk on a + sudden power outage, and may need fsck or journal replays to + be made consistent); on an inactive guest, a disk snapshot is + clean if the disks were clean when the guest was last shut + down.</dd> + <dt>VM state</dt> + <dd>Tracks only the state of RAM and all other resources in use + by the VM. If the disks are unmodified between the time a VM + state snapshot is taken and restored, then the guest will + resume in a consistent state; but if the disks are modified + externally in the meantime, this is likely to lead to data + corruption.</dd> + <dt>system checkpoint</dt> + <dd>A combination of disk snapshots for all disks as well as VM + state, which can be used to resume the guest from where it + left off with symptoms similar to hibernation (that is, TCP + connections in the guest may have timed out, but no files or + processes are lost).</dd> + </dl> + + <p> + Libvirt can manage all three types of snapshots. For now, VM + state snapshots are created only by + the <code>virDomainSave()</code>, <code>virDomainSaveFlags</code>, + and <code>virDomainManagedSave()</code> functions, and restored + via the <code>virDomainRestore()</code>, + <code>virDomainRestoreFlags()</code>, <code>virDomainCreate()</code>, + and <code>virDomainCreateWithFlags()</code> functions (as well + as via domain autostart). With managed snapshots, libvirt + tracks all information internally; with save images, the user + tracks the snapshot file, but libvirt provides functions such + as <code>virDomainSaveImageGetXMLDesc()</code> to work with + those files. + </p> + <p>System checkpoints are created + by <code>virDomainSnapshotCreateXML()</code> with no flags, and + disk snapshots are created by the same function with + the <code>VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY</code> flag; in + both cases, they are restored by + the <code>virDomainRevertToSnapshot()</code> function. For + these types of snapshots, libvirt tracks each snapshot as a + separate <code>virDomainSnapshotPtr</code> object, and maintains + a tree relationship of which snapshots descended from an earlier + point in time. + </p> + + <p> Attributes of libvirt snapshots are stored as child elements of the <code>domainsnapshot</code> element. At snapshot creation time, only the <code>name</code> and <code>description</code> @@ -21,9 +76,10 @@ <dl> <dt><code>name</code></dt> <dd>The name for this snapshot. If the name is specified when - initially creating the snapshot, then the snapshot will have - that particular name. If the name is omitted when initially - creating the snapshot, then libvirt will make up a name for the snapshot. + initially creating the snapshot, then the snapshot will have + that particular name. If the name is omitted when initially + creating the snapshot, then libvirt will make up a name for the + snapshot, based on the time when it was created. </dd> <dt><code>description</code></dt> <dd>A human-readable description of the snapshot. If the @@ -35,15 +91,21 @@ in seconds since the Epoch, UTC (i.e. Unix time). Readonly. </dd> <dt><code>state</code></dt> - <dd>The state of the domain at the time this snapshot was - taken. When the domain is reverted to this snapshot, the domain's state - will be set to whatever is in this field. Readonly. + <dd>The state of the domain at the time this snapshot was taken. + If the snapshot was created as a system checkpoint, then this + is the state of the domain at that time; when the domain is + reverted to this snapshot, the domain's state will default to + whatever is in this field unless additional flags are passed + to <code>virDomainRevertToSnapshot()</code>. Additionally, + this field can be the value "disk-snapshot" when it represents + only a disk snapshot (no VM state), and reverting to this + snapshot will default to an inactive guest. Readonly. </dd> <dt><code>parent</code></dt> - <dd>The parent of this snapshot. This element contains exactly - one child element, name. This specifies the name of the parent - snapshot of this snapshot, and is used to represent trees of - snapshots. Readonly. + <dd>The parent of this snapshot. If present, this element + contains exactly one child element, name. This specifies the + name of the parent snapshot of this snapshot, and is used to + represent trees of snapshots. Readonly. </dd> <dt><code>domain</code></dt> <dd>The domain that this snapshot was taken against. Older diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index a16d731..130dad9 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -22,7 +22,7 @@ </optional> <optional> <element name='state'> - <text/> + <ref name='state'/> </element> </optional> <optional> @@ -59,4 +59,17 @@ </element> </define> + <define name='state'> + <choice> + <value>nostate</value> + <value>running</value> + <value>blocked</value> + <value>paused</value> + <value>shutdown</value> + <value>shutoff</value> + <value>crashed</value> + <value>disk-snapshot</value> + </choice> + </define> + </grammar> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e07dc20..c62577d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -86,7 +86,14 @@ typedef enum { VIR_DOMAIN_PAUSED = 3, /* the domain is paused by user */ VIR_DOMAIN_SHUTDOWN= 4, /* the domain is being shut down */ VIR_DOMAIN_SHUTOFF = 5, /* the domain is shut off */ - VIR_DOMAIN_CRASHED = 6 /* the domain is crashed */ + VIR_DOMAIN_CRASHED = 6, /* the domain is crashed */ + + /* + * NB: this enum value will increase over time as new events are + * added to the libvirt API. It reflects the last state supported + * by this version of the libvirt API. + */ + VIR_DOMAIN_LAST } virDomainState; typedef enum { @@ -1893,6 +1900,11 @@ typedef enum { VIR_KEYCODE_SET_USB = 7, VIR_KEYCODE_SET_WIN32 = 8, + /* + * NB: this enum value will increase over time as new events are + * added to the libvirt API. It reflects the last keycode set supported + * by this version of the libvirt API. + */ VIR_KEYCODE_SET_LAST, } virKeycodeSet; diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5903f65..d5784e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -420,7 +420,7 @@ VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST, "usb", "pci") -VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_CRASHED+1, +VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_LAST, "nostate", "running", "blocked", @@ -429,6 +429,17 @@ VIR_ENUM_IMPL(virDomainState, VIR_DOMAIN_CRASHED+1, "shutoff", "crashed") +/* virDomainSnapshotState is really virDomainState plus one extra state */ +VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_LAST+1, + "nostate", + "running", + "blocked", + "paused", + "shutdown", + "shutoff", + "crashed", + "disk-snapshot") + #define VIR_DOMAIN_NOSTATE_LAST (VIR_DOMAIN_NOSTATE_UNKNOWN + 1) VIR_ENUM_IMPL(virDomainNostateReason, VIR_DOMAIN_NOSTATE_LAST, "unknown") @@ -11052,7 +11063,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("missing state from existing snapshot")); goto cleanup; } - def->state = virDomainStateTypeFromString(state); + def->state = virDomainSnapshotStateTypeFromString(state); if (def->state < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid state '%s' in domain snapshot XML"), @@ -11120,7 +11131,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virBufferAsprintf(&buf, " <description>%s</description>\n", def->description); virBufferAsprintf(&buf, " <state>%s</state>\n", - virDomainStateTypeToString(def->state)); + virDomainSnapshotStateTypeToString(def->state)); if (def->parent) { virBufferAddLit(&buf, " <parent>\n"); virBufferAsprintf(&buf, " <name>%s</name>\n", def->parent); @@ -11712,6 +11723,7 @@ virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) case VIR_DOMAIN_SHUTDOWN: last = VIR_DOMAIN_SHUTDOWN_LAST; break; case VIR_DOMAIN_SHUTOFF: last = VIR_DOMAIN_SHUTOFF_LAST; break; case VIR_DOMAIN_CRASHED: last = VIR_DOMAIN_CRASHED_LAST; break; + default: last = -1; } if (last < 0) { @@ -11745,9 +11757,9 @@ virDomainStateReasonToString(virDomainState state, int reason) return virDomainShutoffReasonTypeToString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeToString(reason); + default: + return NULL; } - - return NULL; } @@ -11769,9 +11781,9 @@ virDomainStateReasonFromString(virDomainState state, const char *reason) return virDomainShutoffReasonTypeFromString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeFromString(reason); + default: + return -1; } - - return -1; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7dbf353..863d64f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1312,7 +1312,7 @@ struct _virDomainSnapshotDef { char *description; char *parent; long long creationTime; /* in seconds */ - int state; + int state; /* enum virDomainSnapshotState */ virDomainDefPtr dom; /* Internal use. */ @@ -1739,6 +1739,7 @@ VIR_ENUM_DECL(virDomainGraphicsSpicePlaybackCompression) VIR_ENUM_DECL(virDomainGraphicsSpiceStreamingMode) VIR_ENUM_DECL(virDomainGraphicsSpiceClipboardCopypaste) VIR_ENUM_DECL(virDomainNumatuneMemMode) +VIR_ENUM_DECL(virDomainSnapshotState) /* from libvirt.h */ VIR_ENUM_DECL(virDomainState) VIR_ENUM_DECL(virDomainNostateReason) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9530567..034443c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -392,6 +392,8 @@ virDomainSnapshotHasChildren; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNum; virDomainSnapshotObjListRemove; +virDomainSnapshotStateTypeFromString; +virDomainSnapshotStateTypeToString; virDomainSoundDefFree; virDomainSoundModelTypeFromString; virDomainSoundModelTypeToString; diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml new file mode 100644 index 0000000..391bb57 --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -0,0 +1,35 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <parent> + <name>earlier_snap</name> + </parent> + <state>disk-snapshot</state> + <creationTime>1272917631</creationTime> +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> + <active>1</active> +</domainsnapshot> diff --git a/tools/virsh.c b/tools/virsh.c index 49ce814..27d0d6f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -920,7 +920,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrint(ctl, "%3d %-20s %s\n", virDomainGetID(dom), virDomainGetName(dom), - vshDomainStateToString(vshDomainState(ctl, dom, NULL))); + _(vshDomainStateToString(vshDomainState(ctl, dom, NULL)))); virDomainFree(dom); } for (i = 0; i < maxname; i++) { @@ -935,7 +935,7 @@ cmdList(vshControl *ctl, const vshCmd *cmd ATTRIBUTE_UNUSED) vshPrint(ctl, "%3s %-20s %s\n", "-", names[i], - vshDomainStateToString(vshDomainState(ctl, dom, NULL))); + _(vshDomainStateToString(vshDomainState(ctl, dom, NULL)))); virDomainFree(dom); VIR_FREE(names[i]); @@ -14521,6 +14521,8 @@ vshDomainState(vshControl *ctl, virDomainPtr dom, int *reason) static const char * vshDomainStateToString(int state) { + /* Can't use virDomainStateTypeToString, because we want to mark + * strings for translation. */ switch ((virDomainState) state) { case VIR_DOMAIN_RUNNING: return N_("running"); @@ -14535,6 +14537,7 @@ vshDomainStateToString(int state) case VIR_DOMAIN_CRASHED: return N_("crashed"); case VIR_DOMAIN_NOSTATE: + default: ;/*FALLTHROUGH*/ } return N_("no state"); /* = dom0 state */ @@ -14636,6 +14639,9 @@ vshDomainStateReasonToString(int state, int reason) ; } break; + + default: + ; } return N_("unknown"); -- 1.7.4.4

Adds an optional element to <domainsnapshot>, which will be used to give user control over external snapshot filenames on input, and specify generated filenames on output. <domainsnapshot> ... <disks> <disk name='vda' snapshot='no'/> <disk name='vdb' snapshot='internal'/> <disk name='vdc' snapshot='external'> <driver type='qcow2'/> <source file='/path/to/new'/> </disk> </disks> <domain> ... <devices> <disk ...> <driver name='qemu' type='raw'/> <target dev='vdc'/> <source file='/path/to/old'/> </disk> </devices> </domain> </domainsnapshot> * src/conf/domain_conf.h (_virDomainSnapshotDiskDef): New type. (_virDomainSnapshotDef): Add new elements. (virDomainSnapshotAlignDisks): New prototype. * src/conf/domain_conf.c (virDomainSnapshotDiskDefClear) (virDomainSnapshotDiskDefParseXML, disksorter) (virDomainSnapshotAlignDisks): New functions. (virDomainSnapshotDefParseString): Parse new fields. (virDomainSnapshotDefFree): Clean them up. (virDomainSnapshotDefFormat): Output them. * src/libvirt_private.syms (domain_conf.h): Export new function. * docs/schemas/domainsnapshot.rng (domainsnapshot, disksnapshot): Add more xml. * docs/formatsnapshot.html.in: Document it. * tests/domainsnapshotxml2xmlin/disk_snapshot.xml: New test. * tests/domainsnapshotxml2xmlout/disk_snapshot.xml: Update. --- Sorry this one's so big in comparison to some of the earlier ones; lots of juicy stuff in here. Still no clients of the new domain_conf features, but that comes next. docs/formatsnapshot.html.in | 123 ++++++++++- docs/schemas/domainsnapshot.rng | 52 +++++ src/conf/domain_conf.c | 239 ++++++++++++++++++++++ src/conf/domain_conf.h | 22 ++- src/libvirt_private.syms | 1 + tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 16 ++ tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 42 ++++ 7 files changed, 484 insertions(+), 11 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlin/disk_snapshot.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 4158a63..5aebd72 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -64,10 +64,10 @@ <p> Attributes of libvirt snapshots are stored as child elements of the <code>domainsnapshot</code> element. At snapshot creation - time, only the <code>name</code> and <code>description</code> - elements are settable; the rest of the fields are informational - (and readonly) and will be filled in by libvirt when the - snapshot is created. + time, only the <code>name</code>, <code>description</code>, + and <code>disks</code> elements are settable; the rest of the + fields are informational (and readonly) and will be filled in by + libvirt when the snapshot is created. </p> <p> The top-level <code>domainsnapshot</code> element may contain @@ -86,6 +86,55 @@ description is omitted when initially creating the snapshot, then this field will be empty. </dd> + <dt><code>disks</code></dt> + <dd>On input, this is an optional listing of specific + instructions for disk snapshots; it is needed when making a + snapshot of only a subset of the disks associated with a + domain, or when overriding the domain defaults for how to + snapshot each disk, or for providing specific control over + what file name is created in an external snapshot. On output, + this is fully populated to show the state of each disk in the + snapshot, including any properties that were generated by the + hypervisor defaults. For system checkpoints, this field is + ignored on input and omitted on output (a system checkpoint + implies that all disks participate in the snapshot process, + and since the current implementation only does internal system + checkpoints, there are no extra details to add); a future + release may allow the use of <code>disks</code> with a system + checkpoint. This element has a list of <code>disk</code> + sub-elements, describing anywhere from zero to all of the + disks associated with the domain. <span class="since">Since + 0.9.5</span> + <dl> + <dt><code>disk</code></dt> + <dd>This sub-element describes the snapshot properties of a + specific disk. The attribute <code>name</code> is + mandatory, and must match the <code><target + dev='name'/></code> of one of + the <a href="formatdomain.html#elementsDisks">disk + devices</a> specified for the domain at the time of the + snapshot. The attribute <code>snapshot</code> is + optional, and has the same values of the disk device + element for a domain + (<code>no</code>, <code>internal</code>, + or <code>external</code>). Some hypervisors like ESX + require that if specified, the snapshot mode must not + override any snapshot mode attached to the corresponding + domain disk, while others like qemu allow this field to + override the domain default. If the snapshot mode is + external (whether specified or inherited), then there is + an optional sub-element <code>source</code>, with an + attribute <code>file</code> giving the name, and an + optional sub-element <code>driver</code>, with an + attribute <code>type</code> giving the driver type (such + as qcow2), of the new file created by the external + snapshot of the new file. Remember that with external + snapshots, the original file name becomes the read-only + snapshot, and the new file name contains the read-write + delta of all disk changes since the snapshot. + </dd> + </dl> + </dd> <dt><code>creationTime</code></dt> <dd>The time this snapshot was created. The time is specified in seconds since the Epoch, UTC (i.e. Unix time). Readonly. @@ -124,14 +173,21 @@ <h2><a name="example">Examples</a></h2> - <p>Using this XML on creation:</p> + <p>Using this XML to create a disk snapshot of just vda on a qemu + domain with two disks:</p> <pre> - <domainsnapshot> - <description>Snapshot of OS install and updates</description> - </domainsnapshot></pre> +<domainsnapshot> + <description>Snapshot of OS install and updates</description> + <disks> + <disk name='vda'> + <source file='/path/to/new'/> + </disk> + <disk name='vdb' snapshot='no'/> + </disks> +</domainsnapshot></pre> - <p>Will result in XML similar to this from - virDomainSnapshotGetXMLDesc:</p> + <p>will result in XML similar to this from + <code>virDomainSnapshotGetXMLDesc()</code>:</p> <pre> <domainsnapshot> <name>1270477159</name> @@ -141,14 +197,61 @@ <parent> <name>bare-os-install</name> </parent> + <disks> + <disk name='vda' snapshot='external'> + <driver type='qcow2'/> + <b><source file='/path/to/new'/></b> + </disk> + <disk name='vdb' snapshot='no'/> + </disks> <domain> <name>fedora</name> <uuid>93a5c045-6457-2c09-e56c-927cdf34e178</uuid> <memory>1048576</memory> ... + <devices> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <b><source file='/path/to/old'/></b> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk' snapshot='external'> + <driver name='qemu' type='raw'/> + <source file='/path/to/old2'/> + <target dev='vdb' bus='virtio'/> + </disk> + ... </devices> </domain> </domainsnapshot></pre> + <p>With that snapshot created, <code>/path/to/old</code> is the + read-only backing file to the new active + file <code>/path/to/new</code>. The <code><domain></code> + element within the snapshot xml records the state of the domain + just before the snapshot; a call + to <code>virDomainGetXMLDesc()</code> will show that the domain + has been changed to reflect the snapshot: + </p> + <pre> +<domain> + <name>fedora</name> + <uuid>93a5c045-6457-2c09-e56c-927cdf34e178</uuid> + <memory>1048576</memory> + ... + <devices> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2'/> + <b><source file='/path/to/new'/></b> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='file' device='disk' snapshot='external'> + <driver name='qemu' type='raw'/> + <source file='/path/to/old2'/> + <target dev='vdb' bus='virtio'/> + </disk> + ... + </devices> +</domain></pre> </body> </html> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 130dad9..671fbe0 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -31,6 +31,13 @@ </element> </optional> <optional> + <element name='disks'> + <zeroOrMore> + <ref name='disksnapshot'/> + </zeroOrMore> + </element> + </optional> + <optional> <element name='active'> <choice> <value>0</value> @@ -72,4 +79,49 @@ </choice> </define> + <define name='disksnapshot'> + <element name='disk'> + <attribute name='name'> + <ref name='deviceName'/> + </attribute> + <choice> + <attribute name='snapshot'> + <value>no</value> + </attribute> + <attribute name='snapshot'> + <value>internal</value> + </attribute> + <group> + <optional> + <attribute name='snapshot'> + <value>external</value> + </attribute> + </optional> + <interleave> + <optional> + <element name='driver'> + <optional> + <attribute name='type'> + <ref name='genericName'/> + </attribute> + </optional> + <empty/> + </element> + </optional> + <optional> + <element name='source'> + <optional> + <attribute name='file'> + <ref name='absFilePath'/> + </attribute> + </optional> + <empty/> + </element> + </optional> + </interleave> + </group> + </choice> + </element> + </define> + </grammar> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0713a25..50a9bb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10987,18 +10987,82 @@ cleanup: } /* Snapshot Def functions */ +static void +virDomainSnapshotDiskDefClear(virDomainSnapshotDiskDefPtr disk) +{ + VIR_FREE(disk->name); + VIR_FREE(disk->file); + VIR_FREE(disk->driverType); +} + void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) { + int i; + if (!def) return; VIR_FREE(def->name); VIR_FREE(def->description); VIR_FREE(def->parent); + for (i = 0; i < def->ndisks; i++) + virDomainSnapshotDiskDefClear(&def->disks[i]); + VIR_FREE(def->disks); virDomainDefFree(def->dom); VIR_FREE(def); } +static int +virDomainSnapshotDiskDefParseXML(xmlNodePtr node, + virDomainSnapshotDiskDefPtr def) +{ + int ret = -1; + char *snapshot = NULL; + xmlNodePtr cur; + + def->name = virXMLPropString(node, "name"); + if (!def->name) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name from disk snapshot element")); + goto cleanup; + } + + snapshot = virXMLPropString(node, "snapshot"); + if (snapshot) { + def->snapshot = virDomainDiskSnapshotTypeFromString(snapshot); + if (def->snapshot <= 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown disk snapshot setting '%s'"), + snapshot); + goto cleanup; + } + } + + cur = node->children; + while (cur) { + if (cur->type == XML_ELEMENT_NODE) { + if (!def->file && + xmlStrEqual(cur->name, BAD_CAST "source")) { + def->file = virXMLPropString(cur, "file"); + } else if (!def->driverType && + xmlStrEqual(cur->name, BAD_CAST "driver")) { + def->driverType = virXMLPropString(cur, "type"); + } + } + cur = cur->next; + } + + if (!def->snapshot && (def->file || def->driverType)) + def->snapshot = VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL; + + ret = 0; +cleanup: + VIR_FREE(snapshot); + if (ret < 0) + virDomainSnapshotDiskDefClear(def); + return ret; +} + /* If newSnapshot is true, caps, expectedVirtTypes, and flags are ignored. */ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, @@ -11011,6 +11075,8 @@ virDomainSnapshotDefParseString(const char *xmlStr, xmlDocPtr xml = NULL; virDomainSnapshotDefPtr def = NULL; virDomainSnapshotDefPtr ret = NULL; + xmlNodePtr *nodes = NULL; + int i; char *creation = NULL, *state = NULL; struct timeval tv; int active; @@ -11044,6 +11110,19 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); + if ((i = virXPathNodeSet("./disks/*)", ctxt, &nodes)) < 0) + goto cleanup; + def->ndisks = i; + if (def->ndisks && VIR_ALLOC_N(def->disks, def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefParseXML(nodes[i], &def->disks[i]) < 0) + goto cleanup; + } + VIR_FREE(nodes); + if (!newSnapshot) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { @@ -11106,6 +11185,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, cleanup: VIR_FREE(creation); VIR_FREE(state); + VIR_FREE(nodes); xmlXPathFreeContext(ctxt); if (ret == NULL) virDomainSnapshotDefFree(def); @@ -11114,12 +11194,143 @@ cleanup: return ret; } +static int +disksorter(const void *a, const void *b) +{ + const virDomainSnapshotDiskDef *diska = a; + const virDomainSnapshotDiskDef *diskb = b; + + return diskb->index - diska->index; +} + +/* Align def->disks to def->domain. Sort the list of def->disks, + * filling in any missing disks or snapshot state defaults given by + * the domain, with a fallback to a passed in default. Issue an error + * and return -1 if any def->disks[n]->name appears more than once or + * does not map to dom->disks. If require_match, also require that + * existing def->disks snapshot states do not override explicit + * def->dom settings. */ +int +virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, + int default_snapshot, + bool require_match) +{ + int ret = -1; + virBitmapPtr map = NULL; + int i; + int j; + bool inuse; + + if (!def->dom) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing domain in snapshot")); + goto cleanup; + } + + if (def->ndisks > def->dom->ndisks) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("too many disk snapshot requests for domain")); + goto cleanup; + } + + /* Unlikely to have a guest without disks but technically possible. */ + if (!def->dom->ndisks) { + ret = 0; + goto cleanup; + } + + if (!(map = virBitmapAlloc(def->dom->ndisks))) { + virReportOOMError(); + goto cleanup; + } + + /* Double check requested disks. */ + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + bool found = false; + + for (j = 0; j < def->dom->ndisks; j++) { + if (STREQ(disk->name, def->dom->disks[j]->dst)) { + int disk_snapshot = def->dom->disks[j]->snapshot; + + if (virBitmapGetBit(map, j, &inuse) < 0 || inuse) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' specified twice"), + disk->name); + goto cleanup; + } + ignore_value(virBitmapSetBit(map, j)); + disk->index = j; + if (!disk_snapshot) + disk_snapshot = default_snapshot; + if (!disk->snapshot) { + disk->snapshot = disk_snapshot; + } else if (disk_snapshot && require_match && + disk->snapshot != disk_snapshot) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("disk '%s' must use snapshot mode " + "'%s'"), disk->name, + virDomainDiskSnapshotTypeToString(disk_snapshot)); + goto cleanup; + } + if (disk->file && + disk->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("file '%s' for disk '%s' requires " + "use of external snapshot mode"), + disk->file, disk->name); + goto cleanup; + } + break; + } + } + if (!found) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("no disk named '%s'"), disk->name); + goto cleanup; + } + } + + /* Provide defaults for all remaining disks. */ + if (VIR_EXPAND_N(def->disks, def->ndisks, + def->dom->ndisks - def->ndisks) < 0) { + virReportOOMError(); + goto cleanup; + } + + for (i = 0; i < def->dom->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk; + + ignore_value(virBitmapGetBit(map, i, &inuse)); + if (inuse) + continue; + disk = &def->disks[def->ndisks++]; + if (!(disk->name = strdup(def->dom->disks[i]->dst))) { + virReportOOMError(); + goto cleanup; + } + disk->index = i; + disk->snapshot = def->dom->disks[i]->snapshot; + if (!disk->snapshot) + disk->snapshot = default_snapshot; + } + + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]), disksorter); + + ret = 0; + +cleanup: + virBitmapFree(map); + return ret; +} + char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal) { virBuffer buf = VIR_BUFFER_INITIALIZER; + int i; virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); @@ -11139,6 +11350,34 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, } virBufferAsprintf(&buf, " <creationTime>%lld</creationTime>\n", def->creationTime); + /* For now, only output <disks> on disk-snapshot */ + if (def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + virBufferAddLit(&buf, " <disks>\n"); + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + if (!disk->name) + continue; + + virBufferEscapeString(&buf, " <disk name='%s'", disk->name); + if (disk->snapshot) + virBufferAsprintf(&buf, " snapshot='%s'", + virDomainDiskSnapshotTypeToString(disk->snapshot)); + if (disk->file || disk->driverType) { + virBufferAddLit(&buf, ">\n"); + if (disk->file) + virBufferEscapeString(&buf, " <source file='%s'/>\n", + disk->file); + if (disk->driverType) + virBufferEscapeString(&buf, " <driver type='%s'/>\n", + disk->driverType); + virBufferAddLit(&buf, " </disk>\n"); + } else { + virBufferAddLit(&buf, "/>\n"); + } + } + virBufferAddLit(&buf, " </disks>\n"); + } if (def->dom) { virDomainDefFormatInternal(def->dom, flags, &buf); } else { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ea8194a..d835f96 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1308,7 +1308,20 @@ enum virDomainTaintFlags { VIR_DOMAIN_TAINT_LAST }; -/* Snapshot state */ +/* Items related to snapshot state */ + +/* Stores disk-snapshot information */ +typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef; +typedef virDomainSnapshotDiskDef *virDomainSnapshotDiskDefPtr; +struct _virDomainSnapshotDiskDef { + char *name; /* name matching the <target dev='...' of the domain */ + int index; /* index within snapshot->dom->disks that matches name */ + int snapshot; /* enum virDomainDiskSnapshot */ + char *file; /* new source file when snapshot is external */ + char *driverType; /* file format type of new file */ +}; + +/* Stores the complete snapshot metadata */ typedef struct _virDomainSnapshotDef virDomainSnapshotDef; typedef virDomainSnapshotDef *virDomainSnapshotDefPtr; struct _virDomainSnapshotDef { @@ -1318,6 +1331,10 @@ struct _virDomainSnapshotDef { char *parent; long long creationTime; /* in seconds */ int state; /* enum virDomainSnapshotState */ + + size_t ndisks; /* should not exceed dom->ndisks */ + virDomainSnapshotDiskDef *disks; + virDomainDefPtr dom; /* Internal use. */ @@ -1351,6 +1368,9 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, unsigned int flags, int internal); +int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot, + int default_snapshot, + bool require_match); virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots, const virDomainSnapshotDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 034443c..a953326 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -381,6 +381,7 @@ virDomainSmartcardDefForeach; virDomainSmartcardDefFree; virDomainSmartcardTypeFromString; virDomainSmartcardTypeToString; +virDomainSnapshotAlignDisks; virDomainSnapshotAssignDef; virDomainSnapshotDefFormat; virDomainSnapshotDefFree; diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml new file mode 100644 index 0000000..1f0beb6 --- /dev/null +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -0,0 +1,16 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <disks> + <disk name='hda'/> + <disk name='hdb' snapshot='no'/> + <disk name='hdc' snapshot='internal'/> + <disk name='hdd' snapshot='external'> + <source/> + <driver type='qed'/> + </disk> + <disk name='hde' snapshot='external'> + <source file='/path/to/new'/> + </disk> + </disks> +</domainsnapshot> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index 391bb57..e0414a1 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -6,6 +6,23 @@ </parent> <state>disk-snapshot</state> <creationTime>1272917631</creationTime> + <disks> + <disk name='hda' snapshot='no'/> + <disk name='hdb' snapshot='no'/> + <disk name='hdc' snapshot='internal'/> + <disk name='hdd' snapshot='external'> + <driver type='qed'/> + <source file='/path/to/generated4'/> + </disk> + <disk name='hde' snapshot='external'> + <driver type='qcow2'/> + <source file='/path/to/new'/> + </disk> + <disk name='hdf' snapshot='external'> + <driver type='qcow2'/> + <source file='/path/to/generated5'/> + </disk> + </disks> <domain type='qemu'> <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> @@ -27,6 +44,31 @@ <target dev='hda' bus='ide'/> <address type='drive' controller='0' bus='0' unit='0'/> </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <address type='drive' controller='0' bus='1' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest3'/> + <target dev='hdc' bus='ide'/> + <address type='drive' controller='0' bus='2' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest4'/> + <target dev='hdd' bus='ide'/> + <address type='drive' controller='0' bus='3' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest5'/> + <target dev='hde' bus='ide'/> + <address type='drive' controller='0' bus='4' unit='0'/> + </disk> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest6'/> + <target dev='hdf' bus='ide'/> + <address type='drive' controller='0' bus='5' unit='0'/> + </disk> <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> -- 1.7.4.4

I got confused when 'virsh domblkinfo dom disk' required the path to a disk (which can be ambiguous, since a single file can back multiple disks), rather than the unambiguous target device name that I was using in disk snapshots. So, in true developer fashion, I went for the best of both worlds - all interfaces that operate on a disk (aka block) now accept either the target name or the unambiguous path to the backing file used by the disk. * src/conf/domain_conf.h (virDomainDiskIndexByName): Add parameter. (virDomainDiskPathByName): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainDiskIndexByName): Also allow searching by path, and decide whether ambiguity is okay. (virDomainDiskPathByName): New function. (virDomainDiskRemoveByName, virDomainSnapshotAlignDisks): Update callers. * src/qemu/qemu_driver.c (qemudDomainBlockPeek) (qemuDomainAttachDeviceConfig, qemuDomainUpdateDeviceConfig) (qemuDomainGetBlockInfo, qemuDiskPathToAlias): Likewise. * src/qemu/qemu_process.c (qemuProcessFindDomainDiskByPath): Likewise. * src/libxl/libxl_driver.c (libxlDomainAttachDeviceDiskLive) (libxlDomainDetachDeviceDiskLive, libxlDomainAttachDeviceConfig) (libxlDomainUpdateDeviceConfig): Likewise. * src/uml/uml_driver.c (umlDomainBlockPeek): Likewise. * src/xen/xend_internal.c (xenDaemonDomainBlockPeek): Likewise. * docs/formatsnapshot.html.in: Update documentation. * tools/virsh.pod (domblkstat, domblkinfo): Likewise. * docs/schemas/domaincommon.rng (diskTarget): Tighten pattern on disk targets. * docs/schemas/domainsnapshot.rng (disksnapshot): Update to match. * tests/domainsnapshotxml2xmlin/disk_snapshot.xml: Update test. --- docs/formatsnapshot.html.in | 7 +- docs/schemas/domaincommon.rng | 7 ++- docs/schemas/domainsnapshot.rng | 5 +- src/conf/domain_conf.c | 57 ++++++++++--- src/conf/domain_conf.h | 4 +- src/libvirt_private.syms | 1 + src/libxl/libxl_driver.c | 11 ++- src/qemu/qemu_driver.c | 104 ++++++++++------------ src/qemu/qemu_process.c | 11 +-- src/uml/uml_driver.c | 56 ++++++------- src/xen/xend_internal.c | 12 +-- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tools/virsh.pod | 8 ++- 13 files changed, 155 insertions(+), 130 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 5aebd72..e476c8f 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -109,8 +109,9 @@ <dt><code>disk</code></dt> <dd>This sub-element describes the snapshot properties of a specific disk. The attribute <code>name</code> is - mandatory, and must match the <code><target - dev='name'/></code> of one of + mandatory, and must match either the <code><target + dev='name'/></code> or an unambiguous <code><source + file='name'/></code> of one of the <a href="formatdomain.html#elementsDisks">disk devices</a> specified for the domain at the time of the snapshot. The attribute <code>snapshot</code> is @@ -179,7 +180,7 @@ <domainsnapshot> <description>Snapshot of OS install and updates</description> <disks> - <disk name='vda'> + <disk name='/path/to/old'> <source file='/path/to/new'/> </disk> <disk name='vdb' snapshot='no'/> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 0af9e0f..e1c6a95 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -770,10 +770,15 @@ </choice> </element> </define> + <define name="diskTarget"> + <data type="string"> + <param name="pattern">(ioemu:)?(fd|hd|sd|vd|xvd|ubd)[a-zA-Z0-9_]+</param> + </data> + </define> <define name="target"> <element name="target"> <attribute name="dev"> - <ref name="deviceName"/> + <ref name="diskTarget"/> </attribute> <optional> <attribute name="bus"> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 671fbe0..0ef0631 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -82,7 +82,10 @@ <define name='disksnapshot'> <element name='disk'> <attribute name='name'> - <ref name='deviceName'/> + <choice> + <ref name='diskTarget'/> + <ref name='absFilePath'/> + </choice> </attribute> <choice> <attribute name='snapshot'> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 043e73f..ec75dbd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5516,17 +5516,44 @@ virDomainChrTargetTypeToString(int deviceType, return type; } -int virDomainDiskIndexByName(virDomainDefPtr def, const char *name) +int +virDomainDiskIndexByName(virDomainDefPtr def, const char *name, + bool allow_ambiguous) { virDomainDiskDefPtr vdisk; int i; + int candidate = -1; + /* We prefer the <target dev='name'/> name (it's shorter, required + * for all disks, and should be unambiguous), but also support + * <source file='name'/> (if unambiguous). Assume dst if there is + * no leading slash, source name otherwise. */ for (i = 0; i < def->ndisks; i++) { vdisk = def->disks[i]; - if (STREQ(vdisk->dst, name)) - return i; + if (*name != '/') { + if (STREQ(vdisk->dst, name)) + return i; + } else if (vdisk->src && + STREQ(vdisk->src, name)) { + if (allow_ambiguous) + return i; + if (candidate >= 0) + return -1; + candidate = i; + } } - return -1; + return candidate; +} + +/* Return the path to a disk image if a string identifies at least one + * disk belonging to the domain (both device strings 'vda' and paths + * '/path/to/file' are converted into '/path/to/file'). */ +const char * +virDomainDiskPathByName(virDomainDefPtr def, const char *name) +{ + int i = virDomainDiskIndexByName(def, name, true); + + return i < 0 ? NULL : def->disks[i]->src; } int virDomainDiskInsert(virDomainDefPtr def, @@ -5602,7 +5629,7 @@ void virDomainDiskRemove(virDomainDefPtr def, size_t i) int virDomainDiskRemoveByName(virDomainDefPtr def, const char *name) { - int i = virDomainDiskIndexByName(def, name); + int i = virDomainDiskIndexByName(def, name, false); if (i < 0) return -1; virDomainDiskRemove(def, i); @@ -11205,11 +11232,12 @@ disksorter(const void *a, const void *b) /* Align def->disks to def->domain. Sort the list of def->disks, * filling in any missing disks or snapshot state defaults given by - * the domain, with a fallback to a passed in default. Issue an error - * and return -1 if any def->disks[n]->name appears more than once or - * does not map to dom->disks. If require_match, also require that - * existing def->disks snapshot states do not override explicit - * def->dom settings. */ + * the domain, with a fallback to a passed in default. Convert paths + * to disk targets for uniformity. Issue an error and return -1 if + * any def->disks[n]->name appears more than once or does not map to + * dom->disks. If require_match, also require that existing + * def->disks snapshot states do not override explicit def->dom + * settings. */ int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, int default_snapshot, @@ -11246,7 +11274,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, /* Double check requested disks. */ for (i = 0; i < def->ndisks; i++) { virDomainSnapshotDiskDefPtr disk = &def->disks[i]; - int idx = virDomainDiskIndexByName(def->dom, disk->name); + int idx = virDomainDiskIndexByName(def->dom, disk->name, false); int disk_snapshot; if (idx < 0) { @@ -11284,6 +11312,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->file, disk->name); goto cleanup; } + if (STRNEQ(disk->file, def->dom->disks[idx]->dst)) { + VIR_FREE(disk->file); + if (!(disk->file = strdup(def->dom->disks[idx]->dst))) { + virReportOOMError(); + goto cleanup; + } + } } /* Provide defaults for all remaining disks. */ diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d835f96..c240fe3 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1557,7 +1557,9 @@ int virDomainVcpuPinAdd(virDomainDefPtr def, int virDomainVcpuPinDel(virDomainDefPtr def, int vcpu); -int virDomainDiskIndexByName(virDomainDefPtr def, const char *name); +int virDomainDiskIndexByName(virDomainDefPtr def, const char *name, + bool allow_ambiguous); +const char *virDomainDiskPathByName(virDomainDefPtr, const char *name); int virDomainDiskInsert(virDomainDefPtr def, virDomainDiskDefPtr disk); void virDomainDiskInsertPreAlloced(virDomainDefPtr def, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a953326..5e8f30c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -284,6 +284,7 @@ virDomainDiskInsert; virDomainDiskInsertPreAlloced; virDomainDiskIoTypeFromString; virDomainDiskIoTypeToString; +virDomainDiskPathByName; virDomainDiskRemove; virDomainDiskRemoveByName; virDomainDiskSnapshotTypeFromString; diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d6e0c28..fe141bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2929,7 +2929,7 @@ libxlDomainAttachDeviceDiskLive(libxlDomainObjPrivatePtr priv, break; case VIR_DOMAIN_DISK_DEVICE_DISK: if (l_disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { - if (virDomainDiskIndexByName(vm->def, l_disk->dst) >= 0) { + if (virDomainDiskIndexByName(vm->def, l_disk->dst, true) >= 0) { libxlError(VIR_ERR_OPERATION_FAILED, _("target %s already exists"), l_disk->dst); goto cleanup; @@ -2991,7 +2991,8 @@ libxlDomainDetachDeviceDiskLive(libxlDomainObjPrivatePtr priv, if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_XEN) { if ((i = virDomainDiskIndexByName(vm->def, - dev->data.disk->dst)) < 0) { + dev->data.disk->dst, + false)) < 0) { libxlError(VIR_ERR_OPERATION_FAILED, _("disk %s not found"), dev->data.disk->dst); goto cleanup; @@ -3061,7 +3062,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) { + if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) { libxlError(VIR_ERR_INVALID_ARG, _("target %s already exists."), disk->dst); return -1; @@ -3172,9 +3173,9 @@ libxlDomainUpdateDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if ((i = virDomainDiskIndexByName(vmdef, disk->dst)) < 0) { + if ((i = virDomainDiskIndexByName(vmdef, disk->dst, false)) < 0) { libxlError(VIR_ERR_INVALID_ARG, - _("target %s doesn't exists."), disk->dst); + _("target %s doesn't exist."), disk->dst); goto cleanup; } orig = vmdef->disks[i]; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 31d2c7c..111214e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5349,7 +5349,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - if (virDomainDiskIndexByName(vmdef, disk->dst) >= 0) { + if (virDomainDiskIndexByName(vmdef, disk->dst, true) >= 0) { qemuReportError(VIR_ERR_INVALID_ARG, _("target %s already exists."), disk->dst); return -1; @@ -5467,10 +5467,10 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef, switch (dev->type) { case VIR_DOMAIN_DEVICE_DISK: disk = dev->data.disk; - pos = virDomainDiskIndexByName(vmdef, disk->dst); + pos = virDomainDiskIndexByName(vmdef, disk->dst, false); if (pos < 0) { qemuReportError(VIR_ERR_INVALID_ARG, - _("target %s doesn't exists."), disk->dst); + _("target %s doesn't exist."), disk->dst); return -1; } orig = vmdef->disks[pos]; @@ -7209,7 +7209,8 @@ qemudDomainBlockPeek (virDomainPtr dom, { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - int fd = -1, ret = -1, i; + int fd = -1, ret = -1; + const char *actual; virCheckFlags(0, -1); @@ -7231,42 +7232,35 @@ qemudDomainBlockPeek (virDomainPtr dom, goto cleanup; } - /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - ret = 0; - break; - } + /* Check the path belongs to this domain. */ + if (!(actual = virDomainDiskPathByName(vm->def, path))) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid path '%s'"), path); + goto cleanup; } + path = actual; - if (ret == 0) { - ret = -1; - /* The path is correct, now try to open it and get its size. */ - fd = open (path, O_RDONLY); - if (fd == -1) { - virReportSystemError(errno, - _("%s: failed to open"), path); - goto cleanup; - } - - /* Seek and read. */ - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || - saferead (fd, buffer, size) == (ssize_t) -1) { - virReportSystemError(errno, - _("%s: failed to seek or read"), path); - goto cleanup; - } + /* The path is correct, now try to open it and get its size. */ + fd = open (path, O_RDONLY); + if (fd == -1) { + virReportSystemError(errno, + _("%s: failed to open"), path); + goto cleanup; + } - ret = 0; - } else { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("invalid path")); + /* Seek and read. */ + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || + saferead (fd, buffer, size) == (ssize_t) -1) { + virReportSystemError(errno, + _("%s: failed to seek or read"), path); + goto cleanup; } + ret = 0; + cleanup: VIR_FORCE_CLOSE(fd); if (vm) @@ -7381,8 +7375,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; - int i; - int format; + int format; /* */ + const char *actual; virCheckFlags(0, -1); @@ -7404,19 +7398,12 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, } /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - disk = vm->def->disks[i]; - break; - } - } - - if (!disk) { + if (!(actual = virDomainDiskPathByName(vm->def, path))) { qemuReportError(VIR_ERR_INVALID_ARG, _("invalid path %s not assigned to domain"), path); goto cleanup; } + path = actual; /* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); @@ -9560,23 +9547,26 @@ static const char * qemuDiskPathToAlias(virDomainObjPtr vm, const char *path) { int i; char *ret = NULL; + virDomainDiskDefPtr disk; - for (i = 0 ; i < vm->def->ndisks ; i++) { - virDomainDiskDefPtr disk = vm->def->disks[i]; + i = virDomainDiskIndexByName(vm->def, path, true); + if (i < 0) + goto cleanup; - if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && - disk->type != VIR_DOMAIN_DISK_TYPE_FILE) - continue; + disk = vm->def->disks[i]; - if (disk->src != NULL && STREQ(disk->src, path)) { - if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) { - virReportOOMError(); - return NULL; - } - break; + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK && + disk->type != VIR_DOMAIN_DISK_TYPE_FILE) + goto cleanup; + + if (disk->src) { + if (virAsprintf(&ret, "drive-%s", disk->info.alias) < 0) { + virReportOOMError(); + return NULL; } } +cleanup: if (!ret) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("No device found for specified path")); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 27214ad..1accfad 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -192,15 +192,10 @@ static virDomainDiskDefPtr qemuProcessFindDomainDiskByPath(virDomainObjPtr vm, const char *path) { - int i; - - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDefPtr disk; + int i = virDomainDiskIndexByName(vm->def, path, true); - disk = vm->def->disks[i]; - if (disk->src != NULL && STREQ(disk->src, path)) - return disk; - } + if (i >= 0) + return vm->def->disks[i]; qemuReportError(VIR_ERR_INTERNAL_ERROR, _("no disk found with path %s"), diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 19b6c55..75c8f27 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -2175,7 +2175,8 @@ umlDomainBlockPeek(virDomainPtr dom, { struct uml_driver *driver = dom->conn->privateData; virDomainObjPtr vm; - int fd = -1, ret = -1, i; + int fd = -1, ret = -1; + const char *actual; virCheckFlags(0, -1); @@ -2196,41 +2197,34 @@ umlDomainBlockPeek(virDomainPtr dom, } /* Check the path belongs to this domain. */ - for (i = 0 ; i < vm->def->ndisks ; i++) { - if (vm->def->disks[i]->src != NULL && - STREQ (vm->def->disks[i]->src, path)) { - ret = 0; - break; - } + if (!(actual = virDomainDiskPathByName(vm->def, path))) { + umlReportError(VIR_ERR_INVALID_ARG, + _("invalid path '%s'"), path); + goto cleanup; } + path = actual; - if (ret == 0) { - ret = -1; - /* The path is correct, now try to open it and get its size. */ - fd = open (path, O_RDONLY); - if (fd == -1) { - virReportSystemError(errno, - _("cannot open %s"), path); - goto cleanup; - } - - /* Seek and read. */ - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should - * be 64 bits on all platforms. - */ - if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || - saferead (fd, buffer, size) == (ssize_t) -1) { - virReportSystemError(errno, - _("cannot read %s"), path); - goto cleanup; - } + /* The path is correct, now try to open it and get its size. */ + fd = open (path, O_RDONLY); + if (fd == -1) { + virReportSystemError(errno, + _("cannot open %s"), path); + goto cleanup; + } - ret = 0; - } else { - umlReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid path")); + /* Seek and read. */ + /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should + * be 64 bits on all platforms. + */ + if (lseek (fd, offset, SEEK_SET) == (off_t) -1 || + saferead (fd, buffer, size) == (ssize_t) -1) { + virReportSystemError(errno, + _("cannot read %s"), path); + goto cleanup; } + ret = 0; + cleanup: VIR_FORCE_CLOSE(fd); if (vm) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 6d5a854..6d62431 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3851,11 +3851,11 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, xenUnifiedPrivatePtr priv; struct sexpr *root = NULL; int fd = -1, ret = -1; - int found = 0, i; virDomainDefPtr def; int id; char * tty; int vncport; + const char *actual; priv = (xenUnifiedPrivatePtr) domain->conn->privateData; @@ -3891,18 +3891,12 @@ xenDaemonDomainBlockPeek (virDomainPtr domain, const char *path, vncport))) goto cleanup; - for (i = 0 ; i < def->ndisks ; i++) { - if (def->disks[i]->src && - STREQ(def->disks[i]->src, path)) { - found = 1; - break; - } - } - if (!found) { + if (!(actual = virDomainDiskPathByName(def, path))) { virXendError(VIR_ERR_INVALID_ARG, _("%s: invalid path"), path); goto cleanup; } + path = actual; /* The path is correct, now try to open it and get its size. */ fd = open (path, O_RDONLY); diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index 1f0beb6..ee6b46a 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -2,7 +2,7 @@ <name>my snap name</name> <description>!@#$%^</description> <disks> - <disk name='hda'/> + <disk name='/dev/HostVG/QEMUGuest1'/> <disk name='hdb' snapshot='no'/> <disk name='hdc' snapshot='internal'/> <disk name='hdd' snapshot='external'> diff --git a/tools/virsh.pod b/tools/virsh.pod index c77b1e4..c37c9e4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -429,7 +429,9 @@ the domain is persistent, these flags are ignored. =item B<domblkstat> I<domain> I<block-device> -Get device block stats for a running domain. +Get device block stats for a running domain. A I<block-device> corresponds +to a unique target name (<target dev='name'/>) or source file (<source +file='name'/>) for one of the disk devices attached to I<domain>. =item B<domifstat> I<domain> I<interface-device> @@ -441,7 +443,9 @@ Get memory stats for a running domain. =item B<domblkinfo> I<domain> I<block-device> -Get block device size info for a domain. +Get block device size info for a domain. A I<block-device> corresponds +to a unique target name (<target dev='name'/>) or source file (<source +file='name'/>) for one of the disk devices attached to I<domain>. =item B<blockpull> I<domain> I<path> [I<bandwidth>] -- 1.7.4.4

Prior to this patch, <domainsnapshot>/<disks> was ignored. This changes it to be an error unless an explicit disk snapshot is requested (a future patch may relax things if it turns out to be useful to have a <disks> specification alongside a system checkpoint). * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY): New flag. * src/libvirt.c (virDomainSnapshotCreateXML): Document it. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Disk snapshots not supported yet. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Likewise. --- include/libvirt/libvirt.h.in | 2 ++ src/esx/esx_driver.c | 6 ++++++ src/libvirt.c | 6 ++++++ src/qemu/qemu_driver.c | 5 +++++ src/vbox/vbox_tmpl.c | 6 ++++++ 5 files changed, 25 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c62577d..440af92 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2574,6 +2574,8 @@ typedef virDomainSnapshot *virDomainSnapshotPtr; typedef enum { VIR_DOMAIN_SNAPSHOT_CREATE_HALT = (1 << 0), /* Stop running guest after snapshot is complete */ + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY = (1 << 1), /* disk snapshot, not + system checkpoint */ } virDomainSnapshotCreateFlags; /* Take a snapshot of the current VM state */ diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 8fbbe8a..14b58c9 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4229,6 +4229,12 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } + if (def->ndisks) { + ESX_ERROR(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots not supported yet")); + return NULL; + } + if (esxVI_LookupVirtualMachineByUuidAndPrepareForTask (priv->primary, domain->uuid, NULL, &virtualMachine, priv->parsedUri->autoAnswer) < 0 || diff --git a/src/libvirt.c b/src/libvirt.c index ffd27bc..c0a17bb 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15482,6 +15482,12 @@ error: * it was active before; otherwise, a running domain will still be * running after the snapshot. This flag is invalid on transient domains. * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, then the + * snapshot will be limited to the disks described in @xmlDesc, and no + * VM state will be saved. For an active guest, the disk image may be + * inconsistent (as if power had been pulled), and specifying this + * with the VIR_DOMAIN_SNAPSHOT_CREATE_HALT flag risks data loss. + * * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure. */ virDomainSnapshotPtr diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 111214e..d0b2132 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8707,6 +8707,11 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; + if (def->ndisks) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots not supported yet")); + goto cleanup; + } if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 0dd5efa..1715432 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5664,6 +5664,12 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1, NULL, 0, 0))) goto cleanup; + if (def->ndisks) { + vboxError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots not supported yet")); + goto cleanup; + } + vboxIIDFromUUID(&domiid, dom->uuid); rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine); if (NS_FAILED(rc)) { -- 1.7.4.4

My RFC for snapshot support [1] proposes several rules for when it is safe to delete or revert to an external snapshot, predicated on the existence of new API flags. These will be incrementally added in future patches, but until then, blindly mishandling a disk snapshot risks corrupting internal state, so it is better to outright reject the attempts until the other pieces are in place, thus incrementally relaxing the restrictions added in this patch. [1] https://www.redhat.com/archives/libvir-list/2011-August/msg00361.html * src/qemu/qemu_driver.c (qemuDomainSnapshotCountExternal): New function. (qemuDomainSnapshotDelete): Use it to add safety valve. (qemuDomainRevertToSnapshot): Add safety valve. --- src/qemu/qemu_driver.c | 31 +++++++++++++++++++++++++++++++ 1 files changed, 31 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d0b2132..ee9d8f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9052,6 +9052,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("revert to external disk snapshots not supported " + "yet")); + goto cleanup; + } + if (vm->current_snapshot) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, @@ -9260,6 +9267,19 @@ qemuDomainSnapshotReparentChildren(void *payload, rep->driver->snapshotDir); } +/* Count how many snapshots in a set have external disk snapshots. */ +static void +qemuDomainSnapshotCountExternal(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr snap = payload; + int *count = data; + + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + (*count)++; +} + static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, unsigned int flags) { @@ -9271,6 +9291,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, struct snap_remove rem; struct snap_reparent rep; bool metadata_only = !!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY); + int external = 0; virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN | VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY | @@ -9293,6 +9314,16 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT || + (virDomainSnapshotForEachDescendant(&vm->snapshots, snap, + qemuDomainSnapshotCountExternal, + &external) && external)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("deletion of external disk snapshots not supported " + "yet")); + goto cleanup; + } + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 1.7.4.4

I still need a good solution for snapshot-create-as to list multiple disk options. But this patch is sufficient to test the creation of disk snapshots using only default <disks> for snapshot-create-as, and snapshot-create provides full access to the snapshot process. * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Add --disk-only. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document it. --- I'm thinking that: virsh snapshot-create-as dom --disk-only --disk-omit vda --disk-internal vdb \ --disk-external vdc --disk-external vdd,type=qcow2,file=/path/to/blah might be a nice syntax for specifying: <disks> <disk name='vda' snapshot='no'/> <disk name='vdb' snapshot='internal'/> <disk name='vdc' snapshot='external'/> <disk name='vdd' snapshot='external'> <driver type='qcow2'/> <source file='/path/to/blah'/> </disk> </disks> but I'm not quite sure how do that without some surgery to virsh parsing functions, since all of the --disk-* options would be allowed more than once. Another thought is: virsh snapshot-create-as dom --disk-only --disk vda,snapshot=no \ --disk vdb,snapshot=internal --disk vdc,snapshot=external \ --disk vdd,snapshot=external,type=qcow2,file=/path/to/blah and having --disk be the only repeated option, at which point it is not too much of a stretch to write: virsh snapshot-create-as dom --disk-only vda,snapshot=no \ vdb,snapshot=internal vdc,snapshot=external \ vdd,snapshot=external,type=qcow2,file=/path/to/blah and from there it looks like existing argv handling. I'll have to play with it more, but I'll save that for a separate patch. tools/virsh.c | 12 ++++++++++-- tools/virsh.pod | 20 +++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b081c49..16e1bf0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12266,6 +12266,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"xmlfile", VSH_OT_DATA, 0, N_("domain snapshot XML")}, {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, + {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {NULL, 0, 0, NULL} }; @@ -12281,6 +12282,9 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "halt")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; + if (vshCommandOptBool(cmd, "disk-only")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12330,6 +12334,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"description", VSH_OT_DATA, 0, N_("description of snapshot")}, {"print-xml", VSH_OT_BOOL, 0, N_("print XML document rather than create")}, {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, + {"disk-only", VSH_OT_BOOL, 0, N_("capture disk state but not vm state")}, {NULL, 0, 0, NULL} }; @@ -12347,6 +12352,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "halt")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; + if (vshCommandOptBool(cmd, "disk-only")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY; + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12374,9 +12382,9 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptBool(cmd, "print-xml")) { - if (vshCommandOptBool(cmd, "halt")) { + if (flags) { vshError(ctl, "%s", - _("--print-xml and --halt are mutually exclusive")); + _("--print-xml does not work with --halt or --disk-only")); goto cleanup; } vshPrint(ctl, "%s\n", buffer); diff --git a/tools/virsh.pod b/tools/virsh.pod index b357330..a702520 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1605,26 +1605,36 @@ used to represent properties of snapshots. =over 4 -=item B<snapshot-create> I<domain> [I<xmlfile>] [I<--halt>] +=item B<snapshot-create> I<domain> [I<xmlfile>] [I<--halt>] [I<--disk-only>] Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. The only properties settable for a domain snapshot are the -<name> and <description>; the rest of the fields are ignored, and +<name> and <description>, as well as <disks> if I<--disk-only> is given; +the rest of the fields are ignored, and automatically filled in by libvirt. If I<xmlfile> is completely omitted, then libvirt will choose a value for all fields. If I<--halt> is specified, the domain will be left in an inactive state after the snapshot is created. -=item B<snapshot-create-as> I<domain> [{I<--print-xml> | I<--halt>}] -[I<name>] [I<description>] +If I<--disk-only> is specified, the snapshot will only include disk +state rather than the usual system checkpoint with vm state. Disk +snapshots are faster than full system checkpoints, but reverting to a +disk snapshot may require fsck or journal replays, since it is like +the disk state at the point when the power cord is abruptly pulled; +and mixing I<--halt> and I<--disk-only> loses any data that was not +flushed to disk at the time. + +=item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--halt>] +[I<--disk-only>]} [I<name>] [I<description>] Create a snapshot for domain I<domain> with the given <name> and <description>; if either value is omitted, libvirt will choose a value. If I<--print-xml> is specified, then XML appropriate for I<snapshot-create> is output, rather than actually creating a snapshot. Otherwise, if I<--halt> is specified, the domain will be left in an -inactive state after the snapshot is created. +inactive state after the snapshot is created, and if I<--disk-only> +is specified, the snapshot will not include vm state. =item B<snapshot-current> I<domain> [I<--name>] [I<--security-info>] -- 1.7.4.4

Lots of earlier patches led up to this point - the qemu snapshot_blkdev monitor command can now be controlled by libvirt! Well, insofar as SELinux doesn't prevent qemu from open(O_CREAT) on the files. There's still some followup work before things work with SELinux enforcing, but this patch is big enough to post now. There's still room for other improvements, too (for example, taking a disk snapshot of an inactive domain, by using qemu-img for both internal and external snapshots; wiring up delete and revert control, including additional flags from my RFC; supporting active QED disk snapshots; supporting per-storage-volume snapshots such as LVM or btrfs snapshots; etc.). But this patch is the one that proves the new XML works! * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Wire in active disk snapshots. (qemuDomainSnapshotDiskPrepare) (qemuDomainSnapshotCreateDiskActive) (qemuDomainSnapshotCreateSingleDiskActive): New functions. --- v2: split per-disk handling into helper function, for easier addition of SELinux handling in later patch src/qemu/qemu_driver.c | 289 +++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 273 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5652e62..85c8e43 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8700,6 +8700,240 @@ cleanup: return ret; } +static int +qemuDomainSnapshotDiskPrepare(virDomainObjPtr vm, virDomainSnapshotDefPtr def) +{ + int ret = -1; + int i; + bool found = false; + bool active = virDomainObjIsActive(vm); + struct stat st; + + for (i = 0; i < def->ndisks; i++) { + virDomainSnapshotDiskDefPtr disk = &def->disks[i]; + + switch (disk->snapshot) { + case VIR_DOMAIN_DISK_SNAPSHOT_INTERNAL: + if (active) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("active qemu domains require external disk " + "snapshots; disk %s requested internal"), + disk->name); + goto cleanup; + } + if (!vm->def->disks[i]->driverType || + STRNEQ(vm->def->disks[i]->driverType, "qcow2")) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("internal snapshot for disk %s unsupported " + "for storage type %s"), + disk->name, + NULLSTR(vm->def->disks[i]->driverType)); + goto cleanup; + } + found = true; + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL: + if (!disk->driverType) { + if (!(disk->driverType = strdup("qcow2"))) { + virReportOOMError(); + goto cleanup; + } + } else if (STRNEQ(disk->driverType, "qcow2")) { + /* XXX We should also support QED */ + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot format for disk %s " + "is unsupported: %s"), + disk->name, disk->driverType); + goto cleanup; + } + if (stat(disk->file, &st) < 0) { + if (errno != ENOENT) { + virReportSystemError(errno, + _("unable to stat for disk %s: %s"), + disk->name, disk->file); + goto cleanup; + } + } else if (!S_ISBLK(st.st_mode)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("external snapshot file for disk %s already " + "exists and is not a block device: %s"), + disk->name, disk->file); + goto cleanup; + } + found = true; + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_NO: + break; + + case VIR_DOMAIN_DISK_SNAPSHOT_DEFAULT: + default: + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected code path")); + goto cleanup; + } + } + + if (!found) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots require at least one disk to be " + "selected for snapshot")); + goto cleanup; + } + + ret = 0; + +cleanup: + return ret; +} + +/* The domain is expected to hold monitor lock. */ +static int +qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, + virDomainSnapshotDiskDefPtr snap, + virDomainDiskDefPtr disk) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *device = NULL; + char *source = NULL; + char *driverType = NULL; + int ret = -1; + + if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { + VIR_ERROR("unexpected code path"); + return -1; + } + + if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0 || + !(source = strdup(snap->file)) || + (STRNEQ_NULLABLE(disk->driverType, "qcow2") && + !(driverType = strdup("qcow2")))) { + virReportOOMError(); + goto cleanup; + } + + /* XXX create new file and set selinux labels */ + ret = qemuMonitorDiskSnapshot(priv->mon, device, source); + virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); + if (ret < 0) + goto cleanup; + + /* Update vm in place to match changes. */ + VIR_FREE(disk->src); + disk->src = source; + source = NULL; + if (driverType) { + VIR_FREE(disk->driverType); + disk->driverType = driverType; + driverType = NULL; + } + + /* XXX Do we also need to update vm->newDef if there are pending + * configuration changes awaiting the next boot? */ + +cleanup: + VIR_FREE(device); + VIR_FREE(source); + VIR_FREE(driverType); + return ret; +} + +/* The domain is expected to be locked and active. */ +static int +qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr *vmptr, + virDomainSnapshotObjPtr snap, + unsigned int flags) +{ + virDomainObjPtr vm = *vmptr; + bool resume = false; + int ret = -1; + int i; + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + return -1; + + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + /* In qemu, snapshot_blkdev on a single disk will pause cpus, + * but this confuses libvirt since notifications are not given + * when qemu resumes. And for multiple disks, libvirt must + * pause externally to get all snapshots to be at the same + * point in time. For simplicitly, we always pause ourselves + * rather than relying on qemu doing pause. + */ + if (qemuProcessStopCPUs(driver, vm, VIR_DOMAIN_PAUSED_SAVE, + QEMU_ASYNC_JOB_NONE) < 0) + goto cleanup; + + resume = true; + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("guest unexpectedly quit")); + goto cleanup; + } + } + + /* No way to roll back if first disk succeeds but later disks + * fail. 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); + for (i = 0; i < snap->def->ndisks; i++) { + if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) + continue; + + ret = qemuDomainSnapshotCreateSingleDiskActive(vm, + &snap->def->disks[i], + vm->def->disks[i]); + if (ret < 0) + break; + } + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) { + virDomainEventPtr event; + + event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); + 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; + if (event) + qemuDomainEventQueue(driver, event); + } + +cleanup: + if (resume && virDomainObjIsActive(vm) && + qemuProcessStartCPUs(driver, vm, conn, + VIR_DOMAIN_RUNNING_UNPAUSED, + QEMU_ASYNC_JOB_NONE) < 0 && + virGetLastError() == NULL) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("resuming after snapshot failed")); + } + + if (vm) { + if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) + ret = -1; + if (qemuDomainObjEndJob(driver, vm) == 0) { + /* 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; + } + } + + return ret; +} + static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, @@ -8713,7 +8947,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_HALT, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_HALT | + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL); qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8742,27 +8977,39 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - /* in a perfect world, we would allow qemu to tell us this. The problem - * is that qemu only does this check device-by-device; so if you had a - * domain that booted from a large qcow2 device, but had a secondary raw - * device attached, you wouldn't find out that you can't snapshot your - * guest until *after* it had spent the time to snapshot the boot device. - * This is probably a bug in qemu, but we'll work around it here for now. - */ - if (!qemuDomainSnapshotIsAllowed(vm)) - goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (virDomainSnapshotAlignDisks(def, VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL, + false) < 0) + goto cleanup; + if (qemuDomainSnapshotDiskPrepare(vm, def) < 0) + goto cleanup; + def->state = VIR_DOMAIN_DISK_SNAPSHOT; + } else { + /* In a perfect world, we would allow qemu to tell us this. + * The problem is that qemu only does this check + * device-by-device; so if you had a domain that booted from a + * large qcow2 device, but had a secondary raw device + * attached, you wouldn't find out that you can't snapshot + * your guest until *after* it had spent the time to snapshot + * the boot device. This is probably a bug in qemu, but we'll + * work around it here for now. + */ + if (!qemuDomainSnapshotIsAllowed(vm)) + goto cleanup; - if (def->ndisks) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots not supported yet")); - goto cleanup; + if (def->ndisks) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots require use of " + "VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY")); + goto cleanup; + } + def->state = virDomainObjGetState(vm, NULL); } if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; - snap->def->state = virDomainObjGetState(vm, NULL); snap->def->current = true; if (vm->current_snapshot) { snap->def->parent = strdup(vm->current_snapshot->def->name); @@ -8778,7 +9025,17 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* actually do the snapshot */ - if (!virDomainObjIsActive(vm)) { + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { + if (!virDomainObjIsActive(vm)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("disk snapshots of inactive domains not " + "implemented yet")); + goto cleanup; + } + if (qemuDomainSnapshotCreateDiskActive(domain->conn, driver, + &vm, snap, flags) < 0) + goto cleanup; + } else if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(driver, vm, snap) < 0) goto cleanup; } else { -- 1.7.4.4

With this, it is now possible to create external snapshots even when SELinux is enforcing, and to protect the new file with a lock manager. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateSingleDiskActive): Create and register new file with proper permissions and locks. (qemuDomainSnapshotCreateDiskActive): Update caller. --- I ran out of time to thoroughly test this today, but wanted to at least get it posted. Once I test it, I'll post the v3 of my series. Oh, and it looks like I don't have any way to conditionally unlink() a just-created file; maybe I should revisit 42/26 to pass yet another bool * parameter that gets updated based on whether a file was created vs. a block device reused. src/qemu/qemu_driver.c | 41 ++++++++++++++++++++++++++++++++++++++--- 1 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c11f08e..cba5929 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -87,6 +87,7 @@ #include "configmake.h" #include "threadpool.h" #include "locking/lock_manager.h" +#include "locking/domain_lock.h" #include "virkeycode.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -8808,7 +8809,8 @@ cleanup: /* The domain is expected to hold monitor lock. */ static int -qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, +qemuDomainSnapshotCreateSingleDiskActive(struct qemud_driver *driver, + virDomainObjPtr vm, virDomainSnapshotDiskDefPtr snap, virDomainDiskDefPtr disk) { @@ -8817,6 +8819,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, char *source = NULL; char *driverType = NULL; int ret = -1; + int fd = -1; + char *origsrc = NULL; + char *origdriver = NULL; if (snap->snapshot != VIR_DOMAIN_DISK_SNAPSHOT_EXTERNAL) { VIR_ERROR("unexpected code path"); @@ -8831,7 +8836,33 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, goto cleanup; } - /* XXX create new file and set selinux labels */ + /* create the stub file and set selinux labels; manipulate disk in + * place, in a way that can be reverted on failure. */ + fd = qemuOpenFile(driver, source, NULL, NULL, false); + if (fd < 0) + goto cleanup; + VIR_FORCE_CLOSE(fd); + + origsrc = disk->src; + disk->src = source; + origdriver = disk->driverType; + disk->driverType = driverType; + + if (virDomainLockDiskAttach(driver->lockManager, vm, disk) < 0) + goto cleanup; + if (virSecurityManagerSetImageLabel(driver->securityManager, vm, + disk) < 0) { + if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) + VIR_WARN("Unable to release lock on %s", source); + goto cleanup; + } + + disk->src = origsrc; + origsrc = NULL; + disk->driverType = driverType; + origdriver = NULL; + + /* create the actual snapshot */ ret = qemuMonitorDiskSnapshot(priv->mon, device, source); virDomainAuditDisk(vm, disk->src, source, "snapshot", ret >= 0); if (ret < 0) @@ -8851,6 +8882,10 @@ qemuDomainSnapshotCreateSingleDiskActive(virDomainObjPtr vm, * configuration changes awaiting the next boot? */ cleanup: + if (origsrc) { + disk->src = origsrc; + disk->driverType = driverType; + } VIR_FREE(device); VIR_FREE(source); VIR_FREE(driverType); @@ -8902,7 +8937,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn, if (snap->def->disks[i].snapshot == VIR_DOMAIN_DISK_SNAPSHOT_NO) continue; - ret = qemuDomainSnapshotCreateSingleDiskActive(vm, + ret = qemuDomainSnapshotCreateSingleDiskActive(driver, vm, &snap->def->disks[i], vm->def->disks[i]); if (ret < 0) -- 1.7.4.4