There are two classes of management apps that track events - one that only cares about on/off (and only needs to track EVENT_STARTED and EVENT_STOPPED), and one that cares about paused/running (also tracks EVENT_SUSPENDED/EVENT_RESUMED). To keep both classes happy, any transition that can go from inactive to paused must emit two back-to-back events - one for started and one for suspended (since later resuming of the domain will only send RESUMED, but the first class isn't tracking that). This also fixes a bug where virDomainCreateWithFlags with the VIR_DOMAIN_START_PAUSED flag failed to start paused when restoring from a managed save image. * include/libvirt/libvirt.h.in (VIR_DOMAIN_EVENT_SUSPENDED_RESTORED) (VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT) (VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT): New sub-events. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Use them. (qemuDomainSaveImageStartVM): Likewise, and add parameter. (qemudDomainCreate, qemuDomainObjStart): Send suspended event when starting paused. (qemuDomainObjRestore): Add parameter. (qemuDomainObjStart, qemuDomainRestoreFlags): Update callers. * examples/domain-events/events-c/event-test.c (eventDetailToString): Map new detail strings. --- examples/domain-events/events-c/event-test.c | 37 +++++++++++++-- include/libvirt/libvirt.h.in | 6 ++- src/qemu/qemu_driver.c | 66 +++++++++++++++++++++----- 3 files changed, 90 insertions(+), 19 deletions(-) diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c index 4766a0d..6a3ed26 100644 --- a/examples/domain-events/events-c/event-test.c +++ b/examples/domain-events/events-c/event-test.c @@ -87,19 +87,45 @@ static const char *eventDetailToString(int event, int detail) { case VIR_DOMAIN_EVENT_STARTED_RESTORED: ret = "Restored"; break; + case VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; } break; case VIR_DOMAIN_EVENT_SUSPENDED: - if (detail == VIR_DOMAIN_EVENT_SUSPENDED_PAUSED) + switch (detail) { + case VIR_DOMAIN_EVENT_SUSPENDED_PAUSED: ret = "Paused"; - else if (detail == VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED) + break; + case VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED: ret = "Migrated"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_IOERROR: + ret = "I/O Error"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_WATCHDOG: + ret = "Watchdog"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_RESTORED: + ret = "Restored"; + break; + case VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; + } break; case VIR_DOMAIN_EVENT_RESUMED: - if (detail == VIR_DOMAIN_EVENT_RESUMED_UNPAUSED) + switch (detail) { + case VIR_DOMAIN_EVENT_RESUMED_UNPAUSED: ret = "Unpaused"; - else if (detail == VIR_DOMAIN_EVENT_RESUMED_MIGRATED) + break; + case VIR_DOMAIN_EVENT_RESUMED_MIGRATED: ret = "Migrated"; + break; + case VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; + } break; case VIR_DOMAIN_EVENT_STOPPED: switch (detail) { @@ -121,6 +147,9 @@ static const char *eventDetailToString(int event, int detail) { case VIR_DOMAIN_EVENT_STOPPED_FAILED: ret = "Failed"; break; + case VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT: + ret = "Snapshot"; + break; } break; } diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e9a1e32..110bd452 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -116,8 +116,7 @@ typedef enum { VIR_DOMAIN_PAUSED_DUMP = 4, /* paused for offline core dump */ VIR_DOMAIN_PAUSED_IOERROR = 5, /* paused due to a disk I/O error */ VIR_DOMAIN_PAUSED_WATCHDOG = 6, /* paused due to a watchdog event */ - VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* restored from a snapshot which was - * taken while domain was paused */ + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT = 7, /* paused after restoring from snapshot */ } virDomainPausedReason; typedef enum { @@ -2029,6 +2028,8 @@ 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_RESTORED = 4, /* Restored from paused state file */ + VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT = 5, /* Restored from paused snapshot */ } virDomainEventSuspendedDetailType; /** @@ -2039,6 +2040,7 @@ typedef enum { typedef enum { VIR_DOMAIN_EVENT_RESUMED_UNPAUSED = 0, /* Normal resume due to admin unpause */ VIR_DOMAIN_EVENT_RESUMED_MIGRATED = 1, /* Resumed for completion of migration */ + VIR_DOMAIN_EVENT_RESUMED_FROM_SNAPSHOT = 2, /* Resumed from snapshot */ } virDomainEventResumedDetailType; /** diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3ff9bd6..9b5d0b2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1269,6 +1269,7 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, virDomainObjPtr vm = NULL; virDomainPtr dom = NULL; virDomainEventPtr event = NULL; + virDomainEventPtr event2 = NULL; virCheckFlags(VIR_DOMAIN_START_PAUSED | VIR_DOMAIN_START_AUTODESTROY, NULL); @@ -1316,6 +1317,16 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_BOOTED); + if (event && (flags & VIR_DOMAIN_START_PAUSED)) { + /* There are two classes of event-watching clients - those + * that only care about on/off (and must see a started event + * no matter what, but don't care about suspend events), and + * those that also care about running/paused. To satisfy both + * client types, we have to send two events. */ + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + } virDomainAuditStart(vm, "booted", true); dom = virGetDomain(conn, vm->def->name, vm->def->uuid); @@ -1329,8 +1340,11 @@ cleanup: virDomainDefFree(def); if (vm) virDomainObjUnlock(vm); - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); + } qemuDriverUnlock(driver); return dom; } @@ -3934,7 +3948,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, virDomainObjPtr vm, int *fd, const struct qemud_save_header *header, - const char *path) + const char *path, + bool start_paused) { int ret = -1; virDomainEventPtr event; @@ -4005,8 +4020,8 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainEventQueue(driver, event); - /* If it was running before, resume it now. */ - if (header->was_running) { + /* If it was running before, resume it now unless caller requested pause. */ + if (header->was_running && !start_paused) { if (qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_RESTORED, QEMU_ASYNC_JOB_NONE) < 0) { @@ -4019,6 +4034,14 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, VIR_WARN("Failed to save status on vm %s", vm->def->name); goto out; } + } else { + int detail = (start_paused ? VIR_DOMAIN_EVENT_SUSPENDED_PAUSED : + VIR_DOMAIN_EVENT_SUSPENDED_RESTORED); + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + if (event) + qemuDomainEventQueue(driver, event); } ret = 0; @@ -4070,7 +4093,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, + false); if (virFileDirectFdClose(directFd) < 0) VIR_WARN("Failed to close %s", path); @@ -4197,6 +4221,7 @@ qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, const char *path, + bool start_paused, bool bypass_cache) { virDomainDefPtr def = NULL; @@ -4230,7 +4255,8 @@ qemuDomainObjRestore(virConnectPtr conn, virDomainObjAssignDef(vm, def, true); def = NULL; - ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, + start_paused); if (virFileDirectFdClose(directFd) < 0) VIR_WARN("Failed to close %s", path); @@ -4518,7 +4544,7 @@ qemuDomainObjStart(virConnectPtr conn, } } else { ret = qemuDomainObjRestore(conn, driver, vm, managed_save, - bypass_cache); + start_paused, bypass_cache); if (ret == 0 && unlink(managed_save) < 0) VIR_WARN("Failed to remove the managed state %s", managed_save); @@ -4537,8 +4563,16 @@ qemuDomainObjStart(virConnectPtr conn, virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_BOOTED); - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (start_paused) { + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_PAUSED); + if (event) + qemuDomainEventQueue(driver, event); + } + } } cleanup: @@ -8806,6 +8840,7 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, virDomainSnapshotObjPtr snap = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainEventPtr event = NULL; + virDomainEventPtr event2 = NULL; qemuDomainObjPrivatePtr priv; int rc; @@ -8862,6 +8897,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); if (snap->def->state == VIR_DOMAIN_PAUSED) { /* qemu unconditionally starts the domain running again after * loadvm, so let's pause it to keep consistency @@ -8872,14 +8910,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, QEMU_ASYNC_JOB_NONE); if (rc < 0) goto endjob; + event2 = 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); } else { /* qemu is a little funny with running guests and the restoration * of snapshots. If the snapshot was taken online, @@ -8922,8 +8959,11 @@ cleanup: } else if (snap) { snap->def->current = false; } - if (event) + if (event) { qemuDomainEventQueue(driver, event); + if (event2) + qemuDomainEventQueue(driver, event2); + } if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- 1.7.4.4

Pretty straight-forward exposure of new flags. For most commands, we let the API reject mutually exclusive flags; but for save-image-edit, we do the sanity check ourselves to avoid looping on flag failure if the edit cycle is ever enhanced to allow the user to retry an edit to fix up an xml validation error. * tools/virsh.c (cmdManagedSave, cmdRestore, cmdSave) (cmdSaveImageDefine, cmdSaveImageEdit): Add new flags. * tools/virsh.pod (managedsave, restore, save, save-image-define) (save-image-edit): Document them. --- tools/virsh.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++----- tools/virsh.pod | 44 +++++++++++++++++++++++++++++++++--- 2 files changed, 99 insertions(+), 10 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d43123c..2e41b45 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -1641,6 +1641,8 @@ static const vshCmdOptDef opts_save[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("where to save the data")}, {"xml", VSH_OT_STRING, 0, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1663,6 +1665,10 @@ cmdSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { vshError(ctl, "%s", _("malformed xml argument")); @@ -1750,6 +1756,8 @@ static const vshCmdOptDef opts_save_image_define[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("saved state file to modify")}, {"xml", VSH_OT_STRING, VSH_OFLAG_REQ, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1760,6 +1768,12 @@ cmdSaveImageDefine(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *xmlfile = NULL; char *xml = NULL; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1799,6 +1813,8 @@ static const vshCmdInfo info_save_image_edit[] = { static const vshCmdOptDef opts_save_image_edit[] = { {"file", VSH_OT_DATA, VSH_OFLAG_REQ, N_("saved state file to edit")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on restore")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on restore")}, {NULL, 0, 0, NULL} }; @@ -1810,7 +1826,22 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) char *tmp = NULL; char *doc = NULL; char *doc_edited = NULL; - unsigned int flags = VIR_DOMAIN_XML_SECURE; + unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; + unsigned int define_flags = 0; + + if (vshCommandOptBool(cmd, "running")) + define_flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + define_flags |= VIR_DOMAIN_SAVE_PAUSED; + + /* Normally, we let the API reject mutually exclusive flags. + * However, in the edit cycle, we let the user retry if the define + * step fails, but the define step will always fail on invalid + * flags, so we reject it up front to avoid looping. */ + if (define_flags == (VIR_DOMAIN_SAVE_RUNNING | VIR_DOMAIN_SAVE_PAUSED)) { + vshError(ctl, "%s", _("--running and --saved are mutually exclusive")); + return false; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -1819,7 +1850,7 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) return false; /* Get the XML configuration of the saved image. */ - doc = virDomainSaveImageGetXMLDesc(ctl->conn, file, flags); + doc = virDomainSaveImageGetXMLDesc(ctl->conn, file, getxml_flags); if (!doc) goto cleanup; @@ -1837,8 +1868,9 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) if (!doc_edited) goto cleanup; - /* Compare original XML with edited. Has it changed at all? */ - if (STREQ(doc, doc_edited)) { + /* Compare original XML with edited. Short-circuit if it did not + * change, and we do not have any flags. */ + if (STREQ(doc, doc_edited) && !define_flags) { vshPrint(ctl, _("Saved image %s XML configuration not changed.\n"), file); ret = true; @@ -1846,7 +1878,8 @@ cmdSaveImageEdit(vshControl *ctl, const vshCmd *cmd) } /* Everything checks out, so redefine the xml. */ - if (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, 0) < 0) { + if (virDomainSaveImageDefineXML(ctl->conn, file, doc_edited, + define_flags) < 0) { vshError(ctl, _("Failed to update %s"), file); goto cleanup; } @@ -1879,6 +1912,8 @@ static const vshCmdInfo info_managedsave[] = { static const vshCmdOptDef opts_managedsave[] = { {"bypass-cache", VSH_OT_BOOL, 0, N_("avoid file system cache when saving")}, {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"running", VSH_OT_BOOL, 0, N_("set domain to be running on next start")}, + {"paused", VSH_OT_BOOL, 0, N_("set domain to be paused on next start")}, {NULL, 0, 0, NULL} }; @@ -1895,6 +1930,10 @@ cmdManagedSave(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) return false; @@ -2238,6 +2277,8 @@ static const vshCmdOptDef opts_restore[] = { N_("avoid file system cache when restoring")}, {"xml", VSH_OT_STRING, 0, N_("filename containing updated XML for the target")}, + {"running", VSH_OT_BOOL, 0, N_("restore domain into running state")}, + {"paused", VSH_OT_BOOL, 0, N_("restore domain into paused state")}, {NULL, 0, 0, NULL} }; @@ -2258,6 +2299,10 @@ cmdRestore(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "bypass-cache")) flags |= VIR_DOMAIN_SAVE_BYPASS_CACHE; + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SAVE_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SAVE_PAUSED; if (vshCommandOptString(cmd, "xml", &xmlfile) < 0) { vshError(ctl, "%s", _("malformed xml argument")); @@ -12382,6 +12427,8 @@ static const vshCmdInfo info_snapshot_revert[] = { static const vshCmdOptDef opts_snapshot_revert[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"running", VSH_OT_BOOL, 0, N_("after reverting, change state to running")}, + {"paused", VSH_OT_BOOL, 0, N_("after reverting, change state to paused")}, {NULL, 0, 0, NULL} }; @@ -12392,6 +12439,12 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *name = NULL; virDomainSnapshotPtr snapshot = NULL; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "running")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING; + if (vshCommandOptBool(cmd, "paused")) + flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12407,7 +12460,7 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd) if (snapshot == NULL) goto cleanup; - if (virDomainRevertToSnapshot(snapshot, 0) < 0) + if (virDomainRevertToSnapshot(snapshot, flags) < 0) goto cleanup; ret = true; diff --git a/tools/virsh.pod b/tools/virsh.pod index b90c26e..30ab5ca 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -623,6 +623,7 @@ The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment variables, and defaults to C<vi>. =item B<managedsave> I<domain-id> [I<--bypass-cache>] +[{I<--running> | I<--paused>}] Save and destroy (stop) a running domain, so it can be restarted from the same state at a later time. When the virsh B<start> command is next run for @@ -630,6 +631,11 @@ the domain, it will automatically be started from this saved state. If I<--bypass-cache> is specified, the save will avoid the file system cache, although this may slow down the operation. +Normally, starting a managed save will decide between running or paused +based on the state the domain was in when the save was done; passing +either the I<--running> or I<--paused> flag will allow overriding which +state the B<start> should use. + The B<dominfo> command can be used to query whether a domain currently has any managed save image. @@ -716,6 +722,7 @@ The exact behavior of a domain when it reboots is set by the I<on_reboot> parameter in the domain's XML definition. =item B<restore> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] +[{I<--running> | I<--paused>}] Restores a domain from a B<virsh save> state file. See I<save> for more info. @@ -728,12 +735,18 @@ in the host-specific portions of the domain XML. For example, it can be used to account for file naming differences in underlying storage due to disk snapshots taken after the guest was saved. +Normally, restoring a saved image will use the state recorded in the +save image to decide between running or paused; passing either the +I<--running> or I<--paused> flag will allow overriding which state the +domain should be started in. + B<Note>: To avoid corrupting file system contents within the domain, you should not reuse the saved state file for a second B<restore> unless you have also reverted all storage volumes back to the same contents as when the state file was created. =item B<save> I<domain-id> I<state-file> [I<--bypass-cache>] [I<--xml> B<file>] +[{I<--running> | I<--paused>}] Saves a running domain (RAM, but not disk state) to a state file so that it can be restored @@ -753,12 +766,17 @@ in the host-specific portions of the domain XML. For example, it can be used to account for file naming differences that are planned to be made via disk snapshots of underlying storage after the guest is saved. +Normally, restoring a saved image will decide between running or paused +based on the state the domain was in when the save was done; passing +either the I<--running> or I<--paused> flag will allow overriding which +state the B<restore> should use. + Domain saved state files assume that disk images will be unchanged between the creation and restore point. For a more complete system restore point, where the disk state is saved alongside the memory state, see the B<snapshot> family of commands. -=item B<save-image-define> I<file> I<xml> +=item B<save-image-define> I<file> I<xml> [{I<--running> | I<--paused>}] Update the domain XML that will be used when I<file> is later used in the B<restore> command. The I<xml> argument must be a file @@ -767,17 +785,27 @@ host-specific portions of the domain XML. For example, it can be used to account for file naming differences resulting from creating disk snapshots of underlying storage after the guest was saved. +The save image records whether the domain should be restored to a +running or paused state. Normally, this command does not alter the +recorded state; passing either the I<--running> or I<--paused> flag +will allow overriding which state the B<restore> should use. + =item B<save-image-dumpxml> I<file> [I<--security-info>] Extract the domain XML that was in effect at the time the saved state file I<file> was created with the B<save> command. Using I<--security-info> will also include security sensitive information. -=item B<save-image-edit> I<file> +=item B<save-image-edit> I<file> [{I<--running> | I<--paused>}] Edit the XML configuration associated with a saved state file I<file> created by the B<save> command. +The save image records whether the domain should be restored to a +running or paused state. Normally, this command does not alter the +recorded state; passing either the I<--running> or I<--paused> flag +will allow overriding which state the B<restore> should use. + This is equivalent to: virsh save-image-dumpxml state-file > state-file.xml @@ -1681,14 +1709,22 @@ Output the snapshot XML for the domain's snapshot named I<snapshot>. Output the name of the parent snapshot for the given I<snapshot>, if any. -=item B<snapshot-revert> I<domain> I<snapshot> +=item B<snapshot-revert> I<domain> I<snapshot> [{I<--running> | I<--paused>}] Revert the given domain to the snapshot specified by I<snapshot>. Be aware -that this is a destructive action; any changes in the domain since the +that this is a destructive action; any changes in the domain since the last 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. +Normally, reverting to a snapshot leaves the domain in the state it was +at the time the snapshot was created, except that a disk snapshot with +no vm state leaves the domain in an inactive state. Passing either the +I<--running> or I<--paused> flag will perform additional state changes +(such as booting an inactive domain, or pausing a running domain). Since +transient domains cannot be inactive, it is required to use one of these +flags when reverting to a disk snapshot of a transient domain. + =item B<snapshot-delete> I<domain> I<snapshot> I<--children> Delete the snapshot for the domain named I<snapshot>. If this snapshot -- 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. Finally, straighten out the events that get emitted on each transition. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Always pause before reversion, and improve events. --- src/qemu/qemu_driver.c | 114 +++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 94 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 914d2be..87888b6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8871,11 +8871,25 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainEventPtr event = NULL; virDomainEventPtr event2 = NULL; + int detail; qemuDomainObjPrivatePtr priv; int rc; virCheckFlags(0, -1); + /* We have the following transitions, which create the following events: + * 1. inactive -> inactive: none + * 2. inactive -> running: EVENT_STARTED + * 3. inactive -> paused: EVENT_STARTED, EVENT_PAUSED + * 4. running -> inactive: EVENT_STOPPED + * 5. running -> running: none + * 6. running -> paused: EVENT_PAUSED + * 7. paused -> inactive: EVENT_STOPPED + * 8. paused -> running: EVENT_RESUMED + * 9. paused -> paused: none + * Also, several transitions occur even if we fail partway through. + */ + qemuDriverLock(driver); virUUIDFormat(snapshot->domain->uuid, uuidstr); vm = virDomainFindByUUID(&driver->domains, snapshot->domain->uuid); @@ -8910,44 +8924,102 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, if (snap->def->state == VIR_DOMAIN_RUNNING || snap->def->state == VIR_DOMAIN_PAUSED) { - + /* Transitions 2, 3, 5, 6, 8, 9 */ + bool was_running = false; + bool was_stopped = false; + + /* 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)) { + /* Transitions 5, 6, 8, 9 */ priv = vm->privateData; + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) { + /* Transitions 5, 6 */ + was_running = true; + 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, we might replace this. */ + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + 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 { + /* Transitions 2, 3 */ + was_stopped = true; rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, - false, false, -1, NULL, snap, + true, false, -1, NULL, snap, VIR_VM_OP_CREATE); virDomainAuditStart(vm, "from-snapshot", rc >= 0); + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); if (rc < 0) goto endjob; } - event = virDomainEventNewFromObj(vm, - VIR_DOMAIN_EVENT_STARTED, - VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT); + /* 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); + /* Transitions 3, 6, 9 */ + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, + VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); + if (was_stopped) { + /* Transition 3, use event as-is and add event2 */ + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + } /* else transition 6 and 9 use event as-is */ + } else { + /* Transitions 2, 5, 8 */ + 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; - event2 = 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); + if (was_stopped) { + /* Transition 2 */ + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + } else if (was_running) { + /* Transition 8 */ + detail = VIR_DOMAIN_EVENT_RESUMED; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_RESUMED, + detail); + } } } else { + /* Transitions 1, 4, 7 */ /* 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 @@ -8959,11 +9031,13 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, */ if (virDomainObjIsActive(vm)) { + /* Transitions 4, 7 */ qemuProcessStop(driver, vm, 0, VIR_DOMAIN_SHUTOFF_FROM_SNAPSHOT); virDomainAuditStop(vm, "from-snapshot"); + detail = VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT; event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_FROM_SNAPSHOT); + detail); if (!vm->persistent) { if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); -- 1.7.4.4

Commit 5e47785 broke reverts to offline system checkpoint snapshots with older qemu, since there is no longer any code path to use qemu -loadvm on next boot. Meanwhile, reverts to offline system checkpoints have been broken for newer qemu, both before and after that commit, since -loadvm no longer works to revert to disk state without accompanying vm state. Fix both of these by using qemu-img to revert disk state. 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 | 64 ++++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 87888b6..66d65c4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8861,6 +8861,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) { @@ -9020,14 +9066,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } } else { /* Transitions 1, 4, 7 */ - /* 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)) { @@ -9039,12 +9080,19 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, VIR_DOMAIN_EVENT_STOPPED, detail); if (!vm->persistent) { + /* XXX For transient domains, enforce that one of the + * flags is set to get domain running again. For now, + * reverting to offline on a transient domain ends up + * killing the domain, without reverting any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; goto cleanup; } } + + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) + goto endjob; } ret = 0; -- 1.7.4.4

For a system checkpoint of a running or paused domain, it's fairly easy to honor new flags for altering which state to use after the revert. For an inactive snapshot, the revert has to be done while there is no qemu process, so do back-to-back transitions; this also lets us revert to inactive snapshots even for transient domains. * src/qemu/qemu_driver.c (qemuDomainRevertToSnapshot): Support new flags. --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66d65c4..d7a2524 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8921,7 +8921,8 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, -1); /* We have the following transitions, which create the following events: * 1. inactive -> inactive: none @@ -8953,6 +8954,17 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto cleanup; } + if (!vm->persistent && + snap->def->state != VIR_DOMAIN_RUNNING && + snap->def->state != VIR_DOMAIN_PAUSED && + (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("transient domain needs to request run or pause " + "to revert to inactive snapshot")); + goto cleanup; + } + if (vm->current_snapshot) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, @@ -9026,7 +9038,9 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, } /* Touch up domain state. */ - if (snap->def->state == VIR_DOMAIN_PAUSED) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) && + (snap->def->state == VIR_DOMAIN_PAUSED || + (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) { /* Transitions 3, 6, 9 */ virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_FROM_SNAPSHOT); @@ -9079,20 +9093,49 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, detail); + } + + if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) { if (!vm->persistent) { - /* XXX For transient domains, enforce that one of the - * flags is set to get domain running again. For now, - * reverting to offline on a transient domain ends up - * killing the domain, without reverting any disk state. */ if (qemuDomainObjEndJob(driver, vm) > 0) virDomainRemoveInactive(&driver->domains, vm); vm = NULL; goto cleanup; } + goto endjob; } - if (qemuDomainSnapshotRevertInactive(vm, snap) < 0) - goto endjob; + if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | + VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { + /* Flush first event, now do transition 2 or 3 */ + bool paused = (flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED) != 0; + + if (event) + qemuDomainEventQueue(driver, event); + rc = qemuProcessStart(snapshot->domain->conn, driver, vm, NULL, + paused, false, -1, NULL, NULL, + VIR_VM_OP_CREATE); + virDomainAuditStart(vm, "from-snapshot", rc >= 0); + if (rc < 0) { + if (!vm->persistent) { + if (qemuDomainObjEndJob(driver, vm) > 0) + virDomainRemoveInactive(&driver->domains, vm); + vm = NULL; + goto cleanup; + } + goto endjob; + } + detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; + event = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_STARTED, + detail); + if (paused) { + detail = VIR_DOMAIN_EVENT_SUSPENDED_FROM_SNAPSHOT; + event2 = virDomainEventNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + detail); + } + } } ret = 0; -- 1.7.4.4

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. * tools/virsh.c (cmdSnapshotList): Add --parent. * tools/virsh.pod (snapshot-list): Document it. --- tools/virsh.c | 39 +++++++++++++++++++++++++++++++++------ tools/virsh.pod | 5 ++++- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2e41b45..b0f4319 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12184,6 +12184,7 @@ 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")}, {NULL, 0, 0, NULL} }; @@ -12192,6 +12193,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) { virDomainPtr dom = NULL; bool ret = false; + unsigned int flags = 0; + int parent_filter = 0; /* 0 for no parent information needed, + 1 for parent column */ int numsnaps; char **names = NULL; int actual = 0; @@ -12201,11 +12205,16 @@ 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")) { + parent_filter = 1; + } + if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12213,19 +12222,25 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; - numsnaps = virDomainSnapshotNum(dom, 0); + 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; @@ -12233,6 +12248,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); @@ -12252,6 +12268,11 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (!xml) continue; + if (parent_filter) { + parent = virXPathString("string(/domainsnapshot/parent/name)", + ctxt); + } + state = virXPathString("string(/domainsnapshot/state)", ctxt); if (state == NULL) continue; @@ -12264,9 +12285,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); } } @@ -12274,6 +12300,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); diff --git a/tools/virsh.pod b/tools/virsh.pod index 30ab5ca..7d8e435 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1697,10 +1697,13 @@ 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. -=item B<snapshot-list> I<domain> +=item B<snapshot-list> I<domain> [I<--parent>] List all of the available snapshots for the given domain. +If I<--parent> is specified, add a column to the output table giving +the name of the parent of each snapshot. + =item B<snapshot-dumpxml> I<domain> I<snapshot> Output the snapshot XML for the domain's snapshot named I<snapshot>. -- 1.7.4.4

This one's nasty. Ever since we fixed virHashForEach to prevent nested hash iterations for safety reasons (commit fba550f6), virDomainSnapshotDelete with VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN has been broken for qemu: it deletes children, while leaving grandchildren intact but pointing to a no-longer-present parent. But even before then, the code would often appear to succeed to clean up grandchildren, but risked memory corruption if you have a large and deep hierarchy of snapshots. For acting on just children, a single virHashForEach is sufficient. But for acting on an entire subtree, it requires iteration; and since we declared recursion as invalid, we have to switch to a while loop. Doing this correctly requires quite a bit of overhaul, so I added a new helper function to isolate the algorithm from the actions, so that callers do not have to reinvent the iteration. Note that this _still_ does not handle CHILDREN correctly if one of the children is the current snapshot; that will be next. * src/conf/domain_conf.h (_virDomainSnapshotDef): Add mark. (virDomainSnapshotForEachDescendant): New prototype. * src/libvirt_private.syms (domain_conf.h): Export it. * src/conf/domain_conf.c (virDomainSnapshotMarkDescendant) (virDomainSnapshotActOnDescendant) (virDomainSnapshotForEachDescendant): New functions. * src/qemu/qemu_driver.c (qemuDomainSnapshotDiscardChildren): Replace... (qemuDomainSnapshotDiscardDescenent): ...with callback that doesn't nest hash traversal. (qemuDomainSnapshotDelete): Use new function. --- src/conf/domain_conf.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 8 +++- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 35 ++++++---------- 4 files changed, 124 insertions(+), 23 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 993d590..f5e2ac9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11256,6 +11256,109 @@ int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, return children.number; } +typedef enum { + MARK_NONE, /* No relation determined yet */ + MARK_DESCENDANT, /* Descendant of target */ + MARK_OTHER, /* Not a descendant of target */ +} snapshot_mark; + +struct snapshot_mark_descendant { + const char *name; /* Parent's name on round 1, NULL on other rounds. */ + virDomainSnapshotObjListPtr snapshots; + bool marked; /* True if descendants were found in this round */ +}; + +/* To be called in a loop until no more descendants are found. + * Additionally marking known unrelated snapshots reduces the number + * of total hash searches needed. */ +static void +virDomainSnapshotMarkDescendant(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + virDomainSnapshotObjPtr obj = payload; + struct snapshot_mark_descendant *curr = data; + virDomainSnapshotObjPtr parent = NULL; + + /* Learned on a previous pass. */ + if (obj->mark) + return; + + if (curr->name) { + /* First round can only find root nodes and direct children. */ + if (!obj->def->parent) { + obj->mark = MARK_OTHER; + } else if (STREQ(obj->def->parent, curr->name)) { + obj->mark = MARK_DESCENDANT; + curr->marked = true; + } + } else { + /* All remaining rounds propagate marks from parents to children. */ + parent = virDomainSnapshotFindByName(curr->snapshots, obj->def->parent); + if (!parent) { + VIR_WARN("snapshot hash table is inconsistent!"); + obj->mark = MARK_OTHER; + return; + } + if (parent->mark) { + obj->mark = parent->mark; + if (obj->mark == MARK_DESCENDANT) + curr->marked = true; + } + } +} + +struct snapshot_act_on_descendant { + int number; + virHashIterator iter; + void *data; +}; + +static void +virDomainSnapshotActOnDescendant(void *payload, + const void *name, + void *data) +{ + virDomainSnapshotObjPtr obj = payload; + struct snapshot_act_on_descendant *curr = data; + + if (obj->mark == MARK_DESCENDANT) { + curr->number++; + (curr->iter)(payload, name, curr->data); + } + obj->mark = MARK_NONE; +} + +/* Run iter(data) on all descendants of snapshot, while ignoring all + * other entries in snapshots. Return the number of descendants + * visited. No particular ordering is guaranteed. */ +int +virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data) +{ + struct snapshot_mark_descendant mark; + struct snapshot_act_on_descendant act; + + /* virHashForEach does not support nested traversal, so we must + * instead iterate until no more snapshots get marked. We + * guarantee that on exit, all marks have been cleared again. */ + mark.name = snapshot->def->name; + mark.snapshots = snapshots; + mark.marked = true; + while (mark.marked) { + mark.marked = false; + virHashForEach(snapshots->objs, virDomainSnapshotMarkDescendant, &mark); + mark.name = NULL; + } + act.number = 0; + act.iter = iter; + act.data = data; + virHashForEach(snapshots->objs, virDomainSnapshotActOnDescendant, &act); + + return act.number; +} int virDomainChrDefForeach(virDomainDefPtr def, bool abortOnError, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 8382d28..d266605 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1139,7 +1139,6 @@ struct _virDomainClockDef { # define VIR_DOMAIN_CPUMASK_LEN 1024 - typedef struct _virDomainVcpuPinDef virDomainVcpuPinDef; typedef virDomainVcpuPinDef *virDomainVcpuPinDefPtr; struct _virDomainVcpuPinDef { @@ -1312,6 +1311,9 @@ typedef struct _virDomainSnapshotObj virDomainSnapshotObj; typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; struct _virDomainSnapshotObj { virDomainSnapshotDefPtr def; + + /* Internal use only */ + int mark; /* Used in identifying descendents. */ }; typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList; @@ -1341,6 +1343,10 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap, virDomainSnapshotObjListPtr snapshots); +int virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr snapshot, + virHashIterator iter, + void *data); /* Guest VM runtime state */ typedef struct _virDomainStateReason virDomainStateReason; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9f03e30..d1dd1d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -384,6 +384,7 @@ virDomainSnapshotDefFormat; virDomainSnapshotDefFree; virDomainSnapshotDefParseString; virDomainSnapshotFindByName; +virDomainSnapshotForEachDescendant; virDomainSnapshotHasChildren; virDomainSnapshotObjListGetNames; virDomainSnapshotObjListNum; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d7a2524..242708c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9257,31 +9257,21 @@ cleanup: struct snap_remove { struct qemud_driver *driver; virDomainObjPtr vm; - char *parent; int err; }; -static void qemuDomainSnapshotDiscardChildren(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) +static void +qemuDomainSnapshotDiscardDescendant(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) { virDomainSnapshotObjPtr snap = payload; struct snap_remove *curr = data; - struct snap_remove this; - - if (snap->def->parent && STREQ(snap->def->parent, curr->parent)) { - this.driver = curr->driver; - this.vm = curr->vm; - this.parent = snap->def->name; - this.err = 0; - virHashForEach(curr->vm->snapshots.objs, - qemuDomainSnapshotDiscardChildren, &this); - - if (this.err) - curr->err = this.err; - else - this.err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap); - } + int err; + + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap); + if (err && !curr->err) + curr->err = err; } struct snap_reparent { @@ -9357,10 +9347,11 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) { rem.driver = driver; rem.vm = vm; - rem.parent = snap->def->name; rem.err = 0; - virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardChildren, - &rem); + virDomainSnapshotForEachDescendant(&vm->snapshots, + snap, + qemuDomainSnapshotDiscardDescendant, + &rem); if (rem.err < 0) goto endjob; } else { -- 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 f5e2ac9..3f3d2bb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11228,32 +11228,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 d1dd1d2..94e28e4 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 a96ec84..85e0f7d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9281,7 +9281,7 @@ qemuDomainSnapshotDiscardDescendant(void *payload, struct snap_reparent { struct qemud_driver *driver; - virDomainSnapshotObjPtr snap; + const char *parent; virDomainObjPtr vm; int err; }; @@ -9298,22 +9298,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, @@ -9364,11 +9362,12 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, vm->current_snapshot = snap; } 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 637145b..38a9058 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2561,6 +2561,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 fdf8368..8870f47 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15672,11 +15672,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) @@ -15712,11 +15720,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 45dd582..dcb3661 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8676,7 +8676,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); @@ -8704,7 +8704,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); @@ -8716,6 +8716,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

The first two flags are essential for being able to replicate snapshot hierarchies across multiple hosts, which will come in handy for supervised migrations. It also allows a management app to take a snapshot of a transient domain, save the metadata, stop the domain, recreate a new transient domain by the same name, redefine the snapshot, then revert to it. This is not quite as convenient as leaving the metadata behind after a domain is no longer around, but doing that has a few problems: 1. the libvirt API can only delete snapshot metadata if there is a valid domain handle to use to get to that snapshot object - if stale data is left behind without a domain, there is no way to request that the data be cleaned up. 2. creating a new domain with the same name but different uuid than the older domain where a snapshot existed cannot use the older snapshot data; this risks confusing libvirt, and forbidding the stale data is similar to the recent patch to forbid stale managed save. The first two flags might be useful on hypervisors with no metadata, but only for modifying the notion of the current snapshot; however, I don't know how to do that for ESX or VBox. The third flag is a convenience option, to combine a creation with a delete metadata into one step. It is trivial for hypervisors with no metadata. The qemu changes will be involved enough to warrant a separate patch. * include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) (VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) (VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA): New flags. * src/libvirt.c (virDomainSnapshotCreateXML): Document them, and enforce mutual exclusion. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Trivial implementation. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * docs/formatsnapshot.html.in: Document re-creation. --- docs/formatsnapshot.html.in | 69 ++++++++++++++++++++++++++---------------- include/libvirt/libvirt.h.in | 9 +++++ src/esx/esx_driver.c | 3 +- src/libvirt.c | 50 +++++++++++++++++++++++++++++- src/vbox/vbox_tmpl.c | 3 +- 5 files changed, 105 insertions(+), 29 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 79ed1d2..e43d192 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -9,10 +9,26 @@ <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, normally only the <code>name</code> + and <code>description</code> elements are settable; the rest of + the fields are ignored on creation, and will be filled in by + libvirt in for informational purposes + by <code>virDomainSnapshotGetXMLDesc()</code>. However, when + redefining a snapshot with + the <code>VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE</code> flag + of <code>virDomainSnapshotCreateXML()</code>, all of the XML + described here is relevant. + </p> + <p> + Snapshots are maintained in a hierarchy. A domain can have a + current snapshot, which is the most recent snapshot compared to + the current state of the domain (although a domain might have + snapshots without a current snapshot, if snapshots have been + deleted in the meantime). Creating or reverting to a snapshot + sets that snapshot as current, and the prior current snapshot is + the parent of the new snapshot. Branches in the hierarchy can + be formed by reverting to a snapshot with a child, then creating + another snapshot. </p> <p> The top-level <code>domainsnapshot</code> element may contain @@ -21,9 +37,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 @@ -32,18 +49,18 @@ </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. + 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. + When the domain is reverted to this snapshot, the domain's + state will default to whatever is in this field. 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. + one child element, name. This specifies the name of the parent + snapshot of this snapshot, and is used to represent trees of + snapshots, as described above. Readonly. </dd> <dt><code>domain</code></dt> <dd>The domain that this snapshot was taken against. This @@ -56,17 +73,17 @@ <h2><a name="example">Example</a></h2> <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> +<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> </body> </html> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 38a9058..00b8350 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2552,6 +2552,15 @@ typedef struct _virDomainSnapshot virDomainSnapshot; */ typedef virDomainSnapshot *virDomainSnapshotPtr; +typedef enum { + VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE = (1 << 0), /* Restore or alter + metadata */ + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT = (1 << 1), /* With redefine, make + snapshot current */ + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA = (1 << 2), /* Make snapshot without + remembering it */ +} virDomainSnapshotCreateFlags; + /* Take a snapshot of the current VM state */ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index dbc7694..1e87664 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4210,7 +4210,8 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, char *taskInfoErrorMessage = NULL; virDomainSnapshotPtr snapshot = NULL; - virCheckFlags(0, NULL); + /* ESX has no snapshot metadata, so this flag is trivial. */ + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); if (esxVI_EnsureSession(priv->primary) < 0) { return NULL; diff --git a/src/libvirt.c b/src/libvirt.c index 8870f47..e3188b7 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15571,11 +15571,46 @@ error: * virDomainSnapshotCreateXML: * @domain: a domain object * @xmlDesc: string containing an XML description of the domain - * @flags: unused flag parameters; callers should pass 0 + * @flags: bitwise-OR of virDomainSnapshotCreateFlags * * Creates a new snapshot of a domain based on the snapshot xml * contained in xmlDesc. * + * If @flags is 0, the domain can be active, in which case the + * snapshot will be a system checkpoint (both disk state and runtime + * VM state such as RAM contents), where reverting to the snapshot is + * the same as resuming from hibernation (TCP connections may have + * timed out, but everything else picks up where it left off); or + * the domain can be inactive, in which case the snapshot includes + * just the disk state prior to booting. The newly created snapshot + * becomes current (see virDomainSnapshotCurrent()), and is a child + * of any previous current snapshot. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, then this + * is a request to reinstate snapshot metadata that was previously + * discarded, rather than creating a new snapshot. This can be used + * to recreate a snapshot hierarchy on a destination, then remove it + * on the source, in order to allow migration (since migration + * normally fails if snapshot metadata still remains on the source + * machine). When redefining snapshot metadata, the current snapshot + * will not be altered unless the VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT + * flag is also present. It is an error to request the + * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT flag without + * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. On some hypervisors, + * redefining an existing snapshot can be used to alter host-specific + * portions of the domain XML to be used during revert (such as + * backing filenames associated with disk devices), but must not alter + * guest-visible layout. When redefining a snapshot name that does + * not exist, the hypervisor may validate that reverting to the + * snapshot appears to be possible (for example, disk images have + * snapshot contents by the requested name). Not all hypervisors + * support these flags. + * + * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, then the + * domain's disk images are modified according to @xmlDesc, but then + * the just-created snapshot has its metadata deleted. This flag is + * incompatible with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. + * * Returns an (opaque) virDomainSnapshotPtr on success, NULL on failure. */ virDomainSnapshotPtr @@ -15607,6 +15642,19 @@ virDomainSnapshotCreateXML(virDomainPtr domain, goto error; } + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT) && + !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("use of current flag requires redefine flag")); + goto error; + } + if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + virLibDomainError(VIR_ERR_INVALID_ARG, + _("redefine and no metadata flags are mutually exclusive")); + goto error; + } + if (conn->driver->domainSnapshotCreateXML) { virDomainSnapshotPtr ret; ret = conn->driver->domainSnapshotCreateXML(domain, xmlDesc, flags); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index fc9739e..afe951c 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5652,7 +5652,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, PRInt32 result; #endif - virCheckFlags(0, NULL); + /* VBox has no snapshot metadata, so this flag is trivial. */ + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) goto cleanup; -- 1.7.4.4

Wire up the new snapshot creation flags in virsh. For convenience, teach 'snapshot-current' how to make an existing snapshot become current (can be used after upgrading to newer libvirt to recover from the fact that the older libvirt lost track of the current snapshot after a restart). The snapshot-create-as command is intentionally not taught --redefine or --current, as this would imply adding a lot of other options for everything else that can appear in the <domainsnapshot> xml, but which is normally read-only. Besides, redefining will usually be done on files created by snapshot-dumpxml, rather than something built up by hand on the command line. And now that we can redefine, we can edit. * tools/virsh.c (cmdSnapshotCreate): Add --redefine, --current, and --no-metadata. (cmdSnapshotCreateAs): Add --no-metadata. (cmdSnapshotCurrent): Add snapshotname to alter current snapshot. (cmdSnapshotEdit): New command. * tools/virsh.pod (snapshot-create, snapshot-create-as) (snapshot-current, snapshot-edit): Document these. --- tools/virsh.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- tools/virsh.pod | 71 ++++++++++++++++++++----- 2 files changed, 215 insertions(+), 18 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index f048dd5..e6a053b 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11921,7 +11921,10 @@ vshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, if (snapshot == NULL) goto cleanup; - doc = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) + doc = vshStrdup(ctl, buffer); + else + doc = virDomainSnapshotGetXMLDesc(snapshot, 0); if (!doc) goto cleanup; @@ -11965,6 +11968,9 @@ static const vshCmdInfo info_snapshot_create[] = { 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")}, + {"redefine", VSH_OT_BOOL, 0, N_("redefine metadata for existing snapshot")}, + {"current", VSH_OT_BOOL, 0, N_("with redefine, set current snapshot")}, + {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, {NULL, 0, 0, NULL} }; @@ -11975,6 +11981,14 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) bool ret = false; const char *from = NULL; char *buffer = NULL; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "redefine")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; + if (vshCommandOptBool(cmd, "current")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; + if (vshCommandOptBool(cmd, "no-metadata")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12000,7 +12014,7 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = vshSnapshotCreate(ctl, dom, buffer, 0, from); + ret = vshSnapshotCreate(ctl, dom, buffer, flags, from); cleanup: VIR_FREE(buffer); @@ -12024,6 +12038,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = { {"name", VSH_OT_DATA, 0, N_("name of snapshot")}, {"description", VSH_OT_DATA, 0, N_("description of snapshot")}, {"print-xml", VSH_OT_BOOL, 0, N_("print XML document rather than create")}, + {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, {NULL, 0, 0, NULL} }; @@ -12036,6 +12051,10 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) const char *name = NULL; const char *desc = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; + unsigned int flags = 0; + + if (vshCommandOptBool(cmd, "no-metadata")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12069,7 +12088,7 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - ret = vshSnapshotCreate(ctl, dom, buffer, 0, NULL); + ret = vshSnapshotCreate(ctl, dom, buffer, flags, NULL); cleanup: VIR_FREE(buffer); @@ -12080,11 +12099,110 @@ cleanup: } /* + * "snapshot-edit" command + */ +static const vshCmdInfo info_snapshot_edit[] = { + {"help", N_("edit XML for a snapshot")}, + {"desc", N_("Edit the domain snapshot XML for a named snapshot")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_snapshot_edit[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"snapshotname", VSH_OT_DATA, VSH_OFLAG_REQ, N_("snapshot name")}, + {"current", VSH_OT_BOOL, 0, N_("also set edited snapshot as current")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + virDomainSnapshotPtr snapshot = NULL; + const char *name; + bool ret = false; + char *tmp = NULL; + char *doc = NULL; + char *doc_edited = NULL; + unsigned int getxml_flags = VIR_DOMAIN_XML_SECURE; + unsigned int define_flags = VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE; + + if (vshCommandOptBool(cmd, "current")) + define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (vshCommandOptString(cmd, "snapshotname", &name) <= 0) + goto cleanup; + + dom = vshCommandOptDomain(ctl, cmd, NULL); + if (dom == NULL) + goto cleanup; + + snapshot = virDomainSnapshotLookupByName(dom, name, 0); + if (snapshot == NULL) + goto cleanup; + + /* Get the XML configuration of the snapshot. */ + doc = virDomainSnapshotGetXMLDesc(snapshot, getxml_flags); + if (!doc) + goto cleanup; + + /* Create and open the temporary file. */ + tmp = editWriteToTempFile(ctl, doc); + if (!tmp) + goto cleanup; + + /* Start the editor. */ + if (editFile(ctl, tmp) == -1) + goto cleanup; + + /* Read back the edited file. */ + doc_edited = editReadBackFile(ctl, tmp); + if (!doc_edited) + goto cleanup; + + /* Compare original XML with edited. Short-circuit if it did not + * change, and we do not have any flags. */ + if (STREQ(doc, doc_edited) && + !(define_flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) { + vshPrint(ctl, _("Snapshot %s XML configuration not changed.\n"), + name); + ret = true; + goto cleanup; + } + + /* Everything checks out, so redefine the xml. */ + snapshot = virDomainSnapshotCreateXML(dom, doc_edited, define_flags); + if (!snapshot) { + vshError(ctl, _("Failed to update %s"), name); + goto cleanup; + } + + vshPrint(ctl, _("Snapshot %s edited.\n"), name); + ret = true; + +cleanup: + VIR_FREE(doc); + VIR_FREE(doc_edited); + if (tmp) { + unlink(tmp); + VIR_FREE(tmp); + } + if (snapshot) + virDomainSnapshotFree(snapshot); + if (dom) + virDomainFree(dom); + return ret; +} + +/* * "snapshot-current" command */ static const vshCmdInfo info_snapshot_current[] = { - {"help", N_("Get the current snapshot")}, - {"desc", N_("Get the current snapshot")}, + {"help", N_("Get or set the current snapshot")}, + {"desc", N_("Get or set the current snapshot")}, {NULL, NULL} }; @@ -12093,6 +12211,8 @@ static const vshCmdOptDef opts_snapshot_current[] = { {"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")}, + {"snapshotname", VSH_OT_DATA, 0, + N_("name of existing snapshot to make current")}, {NULL, 0, 0, NULL} }; @@ -12104,6 +12224,7 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) int current; virDomainSnapshotPtr snapshot = NULL; char *xml = NULL; + const char *snapshotname = NULL; unsigned int flags = 0; if (vshCommandOptBool(cmd, "security-info")) @@ -12116,6 +12237,35 @@ cmdSnapshotCurrent(vshControl *ctl, const vshCmd *cmd) if (dom == NULL) goto cleanup; + if (vshCommandOptString(cmd, "snapshotname", &snapshotname) < 0) { + vshError(ctl, _("invalid snapshotname argument '%s'"), snapshotname); + goto cleanup; + } + if (snapshotname) { + virDomainSnapshotPtr snapshot2 = NULL; + flags = (VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT); + + if (vshCommandOptBool(cmd, "name")) { + vshError(ctl, "%s", + _("--name and snapshotname are mutually exclusive")); + goto cleanup; + } + snapshot = virDomainSnapshotLookupByName(dom, snapshotname, 0); + if (snapshot == NULL) + goto cleanup; + xml = virDomainSnapshotGetXMLDesc(snapshot, VIR_DOMAIN_XML_SECURE); + if (!xml) + goto cleanup; + snapshot2 = virDomainSnapshotCreateXML(dom, xml, flags); + if (snapshot2 == NULL) + goto cleanup; + virDomainSnapshotFree(snapshot2); + vshPrint(ctl, _("Snapshot %s set as current"), snapshotname); + ret = true; + goto cleanup; + } + current = virDomainHasCurrentSnapshot(dom, 0); if (current < 0) goto cleanup; @@ -12950,6 +13100,8 @@ static const vshCmdDef snapshotCmds[] = { info_snapshot_delete, 0}, {"snapshot-dumpxml", cmdSnapshotDumpXML, opts_snapshot_dumpxml, info_snapshot_dumpxml, 0}, + {"snapshot-edit", cmdSnapshotEdit, opts_snapshot_edit, + info_snapshot_edit, 0}, {"snapshot-list", cmdSnapshotList, opts_snapshot_list, info_snapshot_list, 0}, {"snapshot-parent", cmdSnapshotParent, opts_snapshot_parent, diff --git a/tools/virsh.pod b/tools/virsh.pod index cfdfed9..2c8d66c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1675,15 +1675,33 @@ used to represent properties of snapshots. =over 4 -=item B<snapshot-create> I<domain> [I<xmlfile>] +=item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] +| [I<--no-metadata>]} 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 -automatically filled in by libvirt. If I<xmlfile> is completely omitted, -then libvirt will choose a value for all fields. - -=item B<snapshot-create-as> I<domain> [I<--print-xml>] +I<xmlfile>. Normally, the only properties settable for a domain snapshot +are the <name> and <description> elements; 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. +The new snapshot will become current, as listed by B<snapshot-current>. + +If I<--redefine> is specified, then all XML elements produced by +B<snapshot-dumpxml> are valid; this can be used to migrate snapshot +hierarchy from one machine to another, to recreate hierarchy for the +case of a transient domain that goes away and is later recreated with +the same name and UUID, or to make slight alterations in the snapshot +metadata (such as host-specific aspects of the domain XML embedded in +the snapshot). When this flag is supplied, the I<xmlfile> argument +is mandatory, and the domain's current snapshot will not be altered +unless the I<--current> flag is also given. + +If I<--no-metadata> is specified, then the snapshot data is created, +but any metadata is immediately discarded (that is, libvirt does not +treat the snapshot as current, and cannot revert to the snapshot +unless I<--redefine> is later used to teach libvirt about the +metadata again). + +=item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>]} [I<name>] [I<description>] Create a snapshot for domain I<domain> with the given <name> and @@ -1691,13 +1709,40 @@ 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]} +If I<--no-metadata> is specified, then the snapshot data is created, +but any metadata is immediately discarded (that is, libvirt does not +treat the snapshot as current, and cannot revert to the snapshot +unless B<snapshot-create> is later used to teach libvirt about the +metadata again). This flag is incompatible with I<--print-xml>. + +=item B<snapshot-current> I<domain> {[I<--name>] | [I<--security-info] +| [I<snapshotname>]} + +Without I<snapshotname>, this will output the snapshot XML for the domain's +current snapshot (if any). If I<--name> is specified, just the +current snapshot name instead of the full xml. Otherwise, using +I<--security-info> will also include security sensitive information in +the XML. + +With I<snapshotname>, this is a request to make the existing named +snapshot become the current snapshot, without reverting the domain. -Output the snapshot XML for the domain's current snapshot (if any). -If I<--name> is specified, just print the current snapshot name instead -of the full xml. Otherwise, using I<--security-info> will also include -security sensitive information in the XML. +=item B<snapshot-edit> I<domain> I<snapshotname> [I<--current>] + +Edit the XML configuration file for I<snapshotname> of a domain. If +I<--current> is specified, also force the edited snapshot to become +the current snapshot. + +This is equivalent to: + + virsh snapshot-dumpxml dom name > snapshot.xml + vi snapshot.xml (or make changes with your other text editor) + virsh snapshot-create dom snapshot.xml --redefine [--current] + +except that it does some error checking. + +The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment +variables, and defaults to C<vi>. =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots>}] [I<--metadata>] -- 1.7.4.4

Redefining a qemu snapshot requires a bit of a tweak to the common snapshot parsing code, but the end result is quite nice. * src/conf/domain_conf.h (virDomainSnapshotParseFlags): New internal flags. * src/conf/domain_conf.c (virDomainSnapshotDefParseString): Alter signature to take internal flags. * src/esx/esx_driver.c (esxDomainSnapshotCreateXML): Update caller. * src/vbox/vbox_tmpl.c (vboxDomainSnapshotCreateXML): Likewise. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML): Support new public flags. --- src/conf/domain_conf.c | 25 +++++++++++++++------ src/conf/domain_conf.h | 7 +++++- src/esx/esx_driver.c | 2 +- src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++++---------- src/vbox/vbox_tmpl.c | 2 +- 5 files changed, 71 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1b2fd61..009d9c1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10953,8 +10953,9 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) VIR_FREE(def); } -virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot) +/* flags are from virDomainSnapshotParseFlags */ +virDomainSnapshotDefPtr +virDomainSnapshotDefParseString(const char *xmlStr, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; xmlDocPtr xml = NULL; @@ -10982,8 +10983,16 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, gettimeofday(&tv, NULL); def->name = virXPathString("string(./name)", ctxt); - if (def->name == NULL) - ignore_value(virAsprintf(&def->name, "%lld", (long long)tv.tv_sec)); + if (def->name == NULL) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + virDomainReportError(VIR_ERR_XML_ERROR, "%s", + _("a redefined snapshot must have a name")); + goto cleanup; + } else { + ignore_value(virAsprintf(&def->name, "%lld", + (long long)tv.tv_sec)); + } + } if (def->name == NULL) { virReportOOMError(); @@ -10992,7 +11001,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if (!newSnapshot) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -11018,7 +11027,11 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, state); goto cleanup; } + } else { + def->creationTime = tv.tv_sec; + } + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Could not find 'active' element")); @@ -11026,8 +11039,6 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, } def->current = active != 0; } - else - def->creationTime = tv.tv_sec; ret = def; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 503fb58..5e4c67e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1324,8 +1324,13 @@ struct _virDomainSnapshotObjList { virHashTable *objs; }; +typedef enum { + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE = 1 << 0, + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL = 1 << 1, +} virDomainSnapshotParseFlags; + virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, - int newSnapshot); + unsigned int flags); void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def); char *virDomainSnapshotDefFormat(char *domain_uuid, virDomainSnapshotDefPtr def, diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1e87664..e004324 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4217,7 +4217,7 @@ esxDomainSnapshotCreateXML(virDomainPtr domain, const char *xmlDesc, return NULL; } - def = virDomainSnapshotDefParseString(xmlDesc, 1); + def = virDomainSnapshotDefParseString(xmlDesc, 0); if (def == NULL) { return NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6869b02..ac71b04 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -297,6 +297,8 @@ static void qemuDomainSnapshotLoad(void *payload, virDomainSnapshotObjPtr snap = NULL; virDomainSnapshotObjPtr current = NULL; char ebuf[1024]; + unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); virDomainObjLock(vm); if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { @@ -338,7 +340,7 @@ static void qemuDomainSnapshotLoad(void *payload, continue; } - def = virDomainSnapshotDefParseString(xmlStr, 0); + def = virDomainSnapshotDefParseString(xmlStr, flags); if (def == NULL) { /* Nothing we can do here, skip this one */ VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"), @@ -8583,8 +8585,19 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotPtr snapshot = NULL; char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainSnapshotDefPtr def = NULL; + bool update_current = true; + unsigned int parse_flags = 0; - virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | + VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); + + if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && + !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || + (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) + update_current = false; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8611,22 +8624,36 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) goto cleanup; - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, parse_flags))) goto cleanup; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + virDomainSnapshotObjPtr prior = NULL; + + prior = virDomainSnapshotFindByName(&vm->snapshots, def->name); + if (prior) { + /* XXX Ensure ABI compatibility before replacing anything. */ + virDomainSnapshotObjListRemove(&vm->snapshots, prior); + } + } + if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) goto cleanup; def = NULL; - snap->def->state = virDomainObjGetState(vm, NULL); - snap->def->current = true; + if (update_current) + snap->def->current = true; + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) + snap->def->state = virDomainObjGetState(vm, NULL); if (vm->current_snapshot) { - snap->def->parent = strdup(vm->current_snapshot->def->name); - if (snap->def->parent == NULL) { - virReportOOMError(); - goto cleanup; + if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { + snap->def->parent = strdup(vm->current_snapshot->def->name); + if (snap->def->parent == NULL) { + virReportOOMError(); + goto cleanup; + } } - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) { + if (update_current) { vm->current_snapshot->def->current = false; if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot, driver->snapshotDir) < 0) @@ -8636,7 +8663,13 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* actually do the snapshot */ - if (!virDomainObjIsActive(vm)) { + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) { + /* XXX Should we validate that the redefined snapshot even + * makes sense, such as checking whether the requested parent + * snapshot exists and is not creating a loop, or that + * qemu-img recognizes the snapshot name in at least one of + * the domain's disks? */ + } else if (!virDomainObjIsActive(vm)) { if (qemuDomainSnapshotCreateInactive(vm, snap) < 0) goto cleanup; } else { @@ -8658,7 +8691,7 @@ cleanup: driver->snapshotDir) < 0) VIR_WARN("unable to save metadata for snapshot %s", snap->def->name); - else + else if (update_current) vm->current_snapshot = snap; } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index afe951c..636f5f2 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5655,7 +5655,7 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, /* VBox has no snapshot metadata, so this flag is trivial. */ virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL); - if (!(def = virDomainSnapshotDefParseString(xmlDesc, 1))) + if (!(def = virDomainSnapshotDefParseString(xmlDesc, 0))) goto cleanup; vboxIIDFromUUID(&domiid, dom->uuid); -- 1.7.4.4

Just as leaving managed save metadata behind can cause problems when creating a new domain that happens to collide with the name of the just-deleted domain, the same is true of leaving any snapshot metadata behind. For safety sake, extend the semantic change of commit b26a9fa9 to also cover snapshot metadata as a reason to reject undefining an inactive domain. A future patch will make sure that shutdown of a transient domain automatically deletes snapshot metadata (whether by destroy, shutdown, or guest-initiated action). Management apps of transient domains should take care to capture xml of snapshots, if it is necessary to recreate the snapshot metadata on a later transient domain with the same name and uuid. This also documents a new flag that hypervisors can choose to support as a shortcut for taking care of the metadata as part of the undefine process; however, nontrivial driver support for these flags will be deferred to future patches. Note that ESX and VBox can never be transient; therefore, they do not have to worry about automatic cleanup after shutdown (the persistent domain still remains); likewise they never store snapshot metadata, so the undefine flag is trivial. The nontrivial work remaining is thus in the qemu driver. * include/libvirt/libvirt.h.in (VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA): New flag. * src/libvirt.c (virDomainUndefine, virDomainUndefineFlags): Document new limitations and flag. * src/esx/esx_driver.c (esxDomainUndefineFlags): Trivial implementation. * src/vbox/vbox_tmpl.c (vboxDomainUndefineFlags): Likewise. * src/qemu/qemu_driver.c (qemuDomainUndefineFlags): Enforce the limitations. --- include/libvirt/libvirt.h.in | 8 +++++++- src/esx/esx_driver.c | 5 ++++- src/libvirt.c | 35 ++++++++++++++++++++++++++++------- src/qemu/qemu_driver.c | 9 +++++++++ src/vbox/vbox_tmpl.c | 5 ++++- 5 files changed, 52 insertions(+), 10 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 00b8350..4a5dbff 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -923,10 +923,12 @@ virConnectPtr virDomainGetConnect (virDomainPtr domain); * Domain creation and destruction */ + /* * typedef enum { * } virDomainDestroyFlagsValues; */ + virDomainPtr virDomainCreateXML (virConnectPtr conn, const char *xmlDesc, unsigned int flags); @@ -1247,7 +1249,11 @@ virDomainPtr virDomainDefineXML (virConnectPtr conn, int virDomainUndefine (virDomainPtr domain); typedef enum { - VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), + VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = (1 << 0), /* Also remove any + managed save */ + VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA = (1 << 1), /* If last use of domain, + then also remove any + snapshot metadata */ /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index e004324..fb5b1a2 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -3309,7 +3309,10 @@ esxDomainUndefineFlags(virDomainPtr domain, esxVI_String *propertyNameList = NULL; esxVI_VirtualMachinePowerState powerState; - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * ESX, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); if (priv->vCenter != NULL) { ctx = priv->vCenter; diff --git a/src/libvirt.c b/src/libvirt.c index e3188b7..68d469f 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2058,6 +2058,10 @@ error: * does not free the associated virDomainPtr object. * This function may require privileged access * + * If the domain is transient and has any snapshot metadata (see + * virDomainSnapshotNum()), then that metadata will automatically + * be deleted when the domain quits. + * * Returns 0 in case of success and -1 in case of failure. */ int @@ -2107,7 +2111,9 @@ error: * This function may require privileged access. * * Calling this function with no @flags set (equal to zero) - * is equivalent to calling virDomainDestroy. + * is equivalent to calling virDomainDestroy. Using virDomainShutdown() + * may produce cleaner results for the guest's disks, but depends on guest + * support. * * Returns 0 in case of success and -1 in case of failure. */ @@ -2912,12 +2918,17 @@ error: * virDomainShutdown: * @domain: a domain object * - * Shutdown a domain, the domain object is still usable there after but + * Shutdown a domain, the domain object is still usable thereafter but * the domain OS is being stopped. Note that the guest OS may ignore the - * request. + * request. For guests that react to a shutdown request, the differences + * from virDomainDestroy() are that the guests disk storage will be in a + * stable state rather than having the (virtual) power cord pulled, and + * this command returns as soon as the shutdown request is issued rather + * than blocking until the guest is no longer running. * - * TODO: should we add an option for reboot, knowing it may not be doable - * in the general case ? + * If the domain is transient and has any snapshot metadata (see + * virDomainSnapshotNum()), then that metadata will automatically + * be deleted when the domain quits. * * Returns 0 in case of success and -1 in case of failure. */ @@ -6879,8 +6890,9 @@ error: * the domain configuration is removed. * * If the domain has a managed save image (see - * virDomainHasManagedSaveImage()), then the undefine will fail. See - * virDomainUndefineFlags() for more control. + * virDomainHasManagedSaveImage()), or if it is inactive and has any + * snapshot metadata (see virDomainSnapshotNum()), then the undefine will + * fail. See virDomainUndefineFlags() for more control. * * Returns 0 in case of success, -1 in case of error */ @@ -6931,6 +6943,15 @@ error: * then including VIR_DOMAIN_UNDEFINE_MANAGED_SAVE in @flags will also remove * that file, and omitting the flag will cause the undefine process to fail. * + * If the domain is inactive and has any snapshot metadata (see + * virDomainSnapshotNum()), then including + * VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA in @flags will also remove + * that metadata. Omitting the flag will cause the undefine of an + * inactive domain to fail. Active snapshots will retain snapshot + * metadata until the (now-transient) domain halts, regardless of + * whether this flag is present. On hypervisors where snapshots do + * not use libvirt metadata, this flag has no effect. + * * Returns 0 in case of success, -1 in case of error */ int diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ac71b04..38a21db 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4838,6 +4838,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, virDomainEventPtr event = NULL; char *name = NULL; int ret = -1; + int nsnapshots; virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE, -1); @@ -4852,6 +4853,14 @@ qemuDomainUndefineFlags(virDomainPtr dom, goto cleanup; } + if (!virDomainObjIsActive(vm) && + (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d snapshots"), + nsnapshots); + goto cleanup; + } + if (!vm->persistent) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot undefine transient domain")); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 636f5f2..ebed4d9 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -4982,7 +4982,10 @@ vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags) #if VBOX_API_VERSION >= 4000 vboxArray media = VBOX_ARRAY_INITIALIZER; #endif - virCheckFlags(0, -1); + /* No managed save, so we explicitly reject + * VIR_DOMAIN_UNDEFINE_MANAGED_SAVE. No snapshot metadata for + * VBox, so we can trivially ignore that flag. */ + virCheckFlags(VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA, -1); vboxIIDFromUUID(&iid, dom->uuid); -- 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 | 380 ++++++++++++++++++++++++------------------------ 1 files changed, 192 insertions(+), 188 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38a21db..2db0d62 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1582,6 +1582,197 @@ cleanup: } +/* 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 update_current, + 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 (update_current && 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, parentsnap, + 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; + int err; + bool metadata_only; + bool current; +}; + +/* 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; + + if (snap->def->current) + curr->current = true; + err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false, + curr->metadata_only); + if (err && !curr->err) + curr->err = err; +} + static int qemuDomainDestroyFlags(virDomainPtr dom, unsigned int flags) @@ -8391,75 +8582,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; @@ -9219,124 +9341,6 @@ cleanup: return ret; } -static int -qemuDomainSnapshotDiscard(struct qemud_driver *driver, - virDomainObjPtr vm, - virDomainSnapshotObjPtr snap, - bool update_current, - 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 (update_current && 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, parentsnap, - 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; - bool current; -}; - -static void -qemuDomainSnapshotDiscardDescendant(void *payload, - const void *name ATTRIBUTE_UNUSED, - void *data) -{ - virDomainSnapshotObjPtr snap = payload; - struct snap_remove *curr = data; - int err; - - if (snap->def->current) - curr->current = true; - err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false, - curr->metadata_only); - if (err && !curr->err) - curr->err = err; -} - struct snap_reparent { struct qemud_driver *driver; const char *parent; @@ -9415,7 +9419,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rem.current = false; 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. But it is only doable for snapshot metadata, where we are in control of the data being deleted; for the actual snapshots, there's too much likelihood of something going wrong, and requiring even more API calls to figure out what failed in the meantime, so callers are better off deleting the snapshot data themselves one snapshot at a time where they can deal with failures as they happen. * src/qemu/qemu_driver.c (qemuDomainUndefineFlags): Honor new flags. --- src/qemu/qemu_driver.c | 24 +++++++++++++++++++----- 1 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c3a5c1..e5eb8b6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5031,7 +5031,8 @@ 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, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -5046,10 +5047,23 @@ qemuDomainUndefineFlags(virDomainPtr dom, if (!virDomainObjIsActive(vm) && (nsnapshots = virDomainSnapshotObjListNum(&vm->snapshots, 0))) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("cannot delete inactive domain with %d snapshots"), - nsnapshots); - goto cleanup; + struct snap_remove rem; + + if (flags & VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("cannot delete inactive domain with %d " + "snapshots"), + nsnapshots); + goto cleanup; + } + + rem.driver = driver; + rem.vm = vm; + rem.metadata_only = true; + rem.err = 0; + virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardAll, &rem); + if (rem.err < 0) + goto cleanup; } if (!vm->persistent) { -- 1.7.4.4

Minor semantic change - allow domain xml to be generated in place within a larger buffer, rather than having to go through a temporary string. * src/conf/domain_conf.c (virDomainDefFormatInternal): Add parameter. (virDomainDefFormat, virDomainObjFormat): Update callers. --- src/conf/domain_conf.c | 229 ++++++++++++++++++++++++----------------------- 1 files changed, 117 insertions(+), 112 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 009d9c1..bdea5a9 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9903,12 +9903,15 @@ verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | & DUMPXML_FLAGS) == 0); /* This internal version can accept VIR_DOMAIN_XML_INTERNAL_*, - * whereas the public version cannot. */ -static char * + * whereas the public version cannot. Also, it appends to an existing + * buffer, rather than flattening to string. Return -1 on failure. */ +static int virDomainDefFormatInternal(virDomainDefPtr def, - unsigned int flags) + unsigned int flags, + virBufferPtr buf) { - virBuffer buf = VIR_BUFFER_INITIALIZER; + /* XXX Also need to take an indentation parameter - either int or + * string prefix, so that snapshot xml gets uniform indentation. */ unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; @@ -9917,7 +9920,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virCheckFlags(DUMPXML_FLAGS | VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET, - NULL); + -1); if (!(type = virDomainVirtTypeToString(def->virtType))) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -9928,99 +9931,99 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->id == -1) flags |= VIR_DOMAIN_XML_INACTIVE; - virBufferAsprintf(&buf, "<domain type='%s'", type); + virBufferAsprintf(buf, "<domain type='%s'", type); if (!(flags & VIR_DOMAIN_XML_INACTIVE)) - virBufferAsprintf(&buf, " id='%d'", def->id); + virBufferAsprintf(buf, " id='%d'", def->id); if (def->namespaceData && def->ns.href) - virBufferAsprintf(&buf, " %s", (def->ns.href)()); - virBufferAddLit(&buf, ">\n"); + virBufferAsprintf(buf, " %s", (def->ns.href)()); + virBufferAddLit(buf, ">\n"); - virBufferEscapeString(&buf, " <name>%s</name>\n", def->name); + virBufferEscapeString(buf, " <name>%s</name>\n", def->name); uuid = def->uuid; virUUIDFormat(uuid, uuidstr); - virBufferAsprintf(&buf, " <uuid>%s</uuid>\n", uuidstr); + virBufferAsprintf(buf, " <uuid>%s</uuid>\n", uuidstr); if (def->description) - virBufferEscapeString(&buf, " <description>%s</description>\n", + virBufferEscapeString(buf, " <description>%s</description>\n", def->description); - virBufferAsprintf(&buf, " <memory>%lu</memory>\n", def->mem.max_balloon); - virBufferAsprintf(&buf, " <currentMemory>%lu</currentMemory>\n", + virBufferAsprintf(buf, " <memory>%lu</memory>\n", def->mem.max_balloon); + virBufferAsprintf(buf, " <currentMemory>%lu</currentMemory>\n", def->mem.cur_balloon); /* add blkiotune only if there are any */ if (def->blkio.weight) { - virBufferAsprintf(&buf, " <blkiotune>\n"); - virBufferAsprintf(&buf, " <weight>%u</weight>\n", + virBufferAsprintf(buf, " <blkiotune>\n"); + virBufferAsprintf(buf, " <weight>%u</weight>\n", def->blkio.weight); - virBufferAsprintf(&buf, " </blkiotune>\n"); + virBufferAsprintf(buf, " </blkiotune>\n"); } /* add memtune only if there are any */ if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee || def->mem.swap_hard_limit) - virBufferAsprintf(&buf, " <memtune>\n"); + virBufferAsprintf(buf, " <memtune>\n"); if (def->mem.hard_limit) { - virBufferAsprintf(&buf, " <hard_limit>%lu</hard_limit>\n", + virBufferAsprintf(buf, " <hard_limit>%lu</hard_limit>\n", def->mem.hard_limit); } if (def->mem.soft_limit) { - virBufferAsprintf(&buf, " <soft_limit>%lu</soft_limit>\n", + virBufferAsprintf(buf, " <soft_limit>%lu</soft_limit>\n", def->mem.soft_limit); } if (def->mem.min_guarantee) { - virBufferAsprintf(&buf, " <min_guarantee>%lu</min_guarantee>\n", + virBufferAsprintf(buf, " <min_guarantee>%lu</min_guarantee>\n", def->mem.min_guarantee); } if (def->mem.swap_hard_limit) { - virBufferAsprintf(&buf, " <swap_hard_limit>%lu</swap_hard_limit>\n", + virBufferAsprintf(buf, " <swap_hard_limit>%lu</swap_hard_limit>\n", def->mem.swap_hard_limit); } if (def->mem.hard_limit || def->mem.soft_limit || def->mem.min_guarantee || def->mem.swap_hard_limit) - virBufferAsprintf(&buf, " </memtune>\n"); + virBufferAsprintf(buf, " </memtune>\n"); if (def->mem.hugepage_backed) { - virBufferAddLit(&buf, " <memoryBacking>\n"); - virBufferAddLit(&buf, " <hugepages/>\n"); - virBufferAddLit(&buf, " </memoryBacking>\n"); + virBufferAddLit(buf, " <memoryBacking>\n"); + virBufferAddLit(buf, " <hugepages/>\n"); + virBufferAddLit(buf, " </memoryBacking>\n"); } for (n = 0 ; n < def->cpumasklen ; n++) if (def->cpumask[n] != 1) allones = 0; - virBufferAddLit(&buf, " <vcpu"); + virBufferAddLit(buf, " <vcpu"); if (!allones) { char *cpumask = NULL; if ((cpumask = virDomainCpuSetFormat(def->cpumask, def->cpumasklen)) == NULL) goto cleanup; - virBufferAsprintf(&buf, " cpuset='%s'", cpumask); + virBufferAsprintf(buf, " cpuset='%s'", cpumask); VIR_FREE(cpumask); } if (def->vcpus != def->maxvcpus) - virBufferAsprintf(&buf, " current='%u'", def->vcpus); - virBufferAsprintf(&buf, ">%u</vcpu>\n", def->maxvcpus); + virBufferAsprintf(buf, " current='%u'", def->vcpus); + virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); if (def->cputune.shares || def->cputune.vcpupin || def->cputune.period || def->cputune.quota) - virBufferAddLit(&buf, " <cputune>\n"); + virBufferAddLit(buf, " <cputune>\n"); if (def->cputune.shares) - virBufferAsprintf(&buf, " <shares>%lu</shares>\n", + virBufferAsprintf(buf, " <shares>%lu</shares>\n", def->cputune.shares); if (def->cputune.period) - virBufferAsprintf(&buf, " <period>%llu</period>\n", + virBufferAsprintf(buf, " <period>%llu</period>\n", def->cputune.period); if (def->cputune.quota) - virBufferAsprintf(&buf, " <quota>%lld</quota>\n", + virBufferAsprintf(buf, " <quota>%lld</quota>\n", def->cputune.quota); if (def->cputune.vcpupin) { int i; for (i = 0; i < def->cputune.nvcpupin; i++) { - virBufferAsprintf(&buf, " <vcpupin vcpu='%u' ", + virBufferAsprintf(buf, " <vcpupin vcpu='%u' ", def->cputune.vcpupin[i]->vcpuid); char *cpumask = NULL; @@ -10033,17 +10036,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto cleanup; } - virBufferAsprintf(&buf, "cpuset='%s'/>\n", cpumask); + virBufferAsprintf(buf, "cpuset='%s'/>\n", cpumask); VIR_FREE(cpumask); } } if (def->cputune.shares || def->cputune.vcpupin || def->cputune.period || def->cputune.quota) - virBufferAddLit(&buf, " </cputune>\n"); + virBufferAddLit(buf, " </cputune>\n"); if (def->numatune.memory.nodemask) - virBufferAddLit(&buf, " <numatune>\n"); + virBufferAddLit(buf, " <numatune>\n"); if (def->numatune.memory.nodemask) { char *nodemask = NULL; @@ -10055,32 +10058,32 @@ virDomainDefFormatInternal(virDomainDefPtr def, goto cleanup; } - virBufferAsprintf(&buf, " <memory mode='%s' nodeset='%s'/>\n", + virBufferAsprintf(buf, " <memory mode='%s' nodeset='%s'/>\n", virDomainNumatuneMemModeTypeToString(def->numatune.memory.mode), nodemask); VIR_FREE(nodemask); } if (def->numatune.memory.nodemask) - virBufferAddLit(&buf, " </numatune>\n"); + virBufferAddLit(buf, " </numatune>\n"); if (def->sysinfo) - virDomainSysinfoDefFormat(&buf, def->sysinfo); + virDomainSysinfoDefFormat(buf, def->sysinfo); if (def->os.bootloader) { - virBufferEscapeString(&buf, " <bootloader>%s</bootloader>\n", + virBufferEscapeString(buf, " <bootloader>%s</bootloader>\n", def->os.bootloader); if (def->os.bootloaderArgs) - virBufferEscapeString(&buf, " <bootloader_args>%s</bootloader_args>\n", + virBufferEscapeString(buf, " <bootloader_args>%s</bootloader_args>\n", def->os.bootloaderArgs); } - virBufferAddLit(&buf, " <os>\n"); + virBufferAddLit(buf, " <os>\n"); - virBufferAddLit(&buf, " <type"); + virBufferAddLit(buf, " <type"); if (def->os.arch) - virBufferAsprintf(&buf, " arch='%s'", def->os.arch); + virBufferAsprintf(buf, " arch='%s'", def->os.arch); if (def->os.machine) - virBufferAsprintf(&buf, " machine='%s'", def->os.machine); + virBufferAsprintf(buf, " machine='%s'", def->os.machine); /* * HACK: For xen driver we previously used bogus 'linux' as the * os type for paravirt, whereas capabilities declare it to @@ -10088,27 +10091,27 @@ virDomainDefFormatInternal(virDomainDefPtr def, */ if (def->virtType == VIR_DOMAIN_VIRT_XEN && STREQ(def->os.type, "xen")) - virBufferAsprintf(&buf, ">%s</type>\n", "linux"); + virBufferAsprintf(buf, ">%s</type>\n", "linux"); else - virBufferAsprintf(&buf, ">%s</type>\n", def->os.type); + virBufferAsprintf(buf, ">%s</type>\n", def->os.type); if (def->os.init) - virBufferEscapeString(&buf, " <init>%s</init>\n", + virBufferEscapeString(buf, " <init>%s</init>\n", def->os.init); if (def->os.loader) - virBufferEscapeString(&buf, " <loader>%s</loader>\n", + virBufferEscapeString(buf, " <loader>%s</loader>\n", def->os.loader); if (def->os.kernel) - virBufferEscapeString(&buf, " <kernel>%s</kernel>\n", + virBufferEscapeString(buf, " <kernel>%s</kernel>\n", def->os.kernel); if (def->os.initrd) - virBufferEscapeString(&buf, " <initrd>%s</initrd>\n", + virBufferEscapeString(buf, " <initrd>%s</initrd>\n", def->os.initrd); if (def->os.cmdline) - virBufferEscapeString(&buf, " <cmdline>%s</cmdline>\n", + virBufferEscapeString(buf, " <cmdline>%s</cmdline>\n", def->os.cmdline); if (def->os.root) - virBufferEscapeString(&buf, " <root>%s</root>\n", + virBufferEscapeString(buf, " <root>%s</root>\n", def->os.root); if (!def->os.bootloader) { @@ -10121,21 +10124,21 @@ virDomainDefFormatInternal(virDomainDefPtr def, def->os.bootDevs[n]); goto cleanup; } - virBufferAsprintf(&buf, " <boot dev='%s'/>\n", boottype); + virBufferAsprintf(buf, " <boot dev='%s'/>\n", boottype); } if (def->os.bootmenu != VIR_DOMAIN_BOOT_MENU_DEFAULT) { const char *enabled = (def->os.bootmenu == VIR_DOMAIN_BOOT_MENU_ENABLED ? "yes" : "no"); - virBufferAsprintf(&buf, " <bootmenu enable='%s'/>\n", enabled); + virBufferAsprintf(buf, " <bootmenu enable='%s'/>\n", enabled); } if (def->os.bios.useserial) { const char *useserial = (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES ? "yes" : "no"); - virBufferAsprintf(&buf, " <bios useserial='%s'/>\n", useserial); + virBufferAsprintf(buf, " <bios useserial='%s'/>\n", useserial); } } @@ -10148,14 +10151,14 @@ virDomainDefFormatInternal(virDomainDefPtr def, _("unexpected smbios mode %d"), def->os.smbios_mode); goto cleanup; } - virBufferAsprintf(&buf, " <smbios mode='%s'/>\n", mode); + virBufferAsprintf(buf, " <smbios mode='%s'/>\n", mode); } - virBufferAddLit(&buf, " </os>\n"); + virBufferAddLit(buf, " </os>\n"); if (def->features) { int i; - virBufferAddLit(&buf, " <features>\n"); + virBufferAddLit(buf, " <features>\n"); for (i = 0 ; i < VIR_DOMAIN_FEATURE_LAST ; i++) { if (def->features & (1 << i)) { const char *name = virDomainFeatureTypeToString(i); @@ -10164,91 +10167,91 @@ virDomainDefFormatInternal(virDomainDefPtr def, _("unexpected feature %d"), i); goto cleanup; } - virBufferAsprintf(&buf, " <%s/>\n", name); + virBufferAsprintf(buf, " <%s/>\n", name); } } - virBufferAddLit(&buf, " </features>\n"); + virBufferAddLit(buf, " </features>\n"); } - if (virCPUDefFormatBuf(&buf, def->cpu, " ", 0) < 0) + if (virCPUDefFormatBuf(buf, def->cpu, " ", 0) < 0) goto cleanup; - virBufferAsprintf(&buf, " <clock offset='%s'", + virBufferAsprintf(buf, " <clock offset='%s'", virDomainClockOffsetTypeToString(def->clock.offset)); switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_VARIABLE: - virBufferAsprintf(&buf, " adjustment='%lld'", def->clock.data.adjustment); + virBufferAsprintf(buf, " adjustment='%lld'", def->clock.data.adjustment); break; case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: - virBufferEscapeString(&buf, " timezone='%s'", def->clock.data.timezone); + virBufferEscapeString(buf, " timezone='%s'", def->clock.data.timezone); break; } if (def->clock.ntimers == 0) { - virBufferAddLit(&buf, "/>\n"); + virBufferAddLit(buf, "/>\n"); } else { - virBufferAddLit(&buf, ">\n"); + virBufferAddLit(buf, ">\n"); for (n = 0; n < def->clock.ntimers; n++) { - if (virDomainTimerDefFormat(&buf, def->clock.timers[n]) < 0) + if (virDomainTimerDefFormat(buf, def->clock.timers[n]) < 0) goto cleanup; } - virBufferAddLit(&buf, " </clock>\n"); + virBufferAddLit(buf, " </clock>\n"); } - if (virDomainLifecycleDefFormat(&buf, def->onPoweroff, + if (virDomainLifecycleDefFormat(buf, def->onPoweroff, "on_poweroff", virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(&buf, def->onReboot, + if (virDomainLifecycleDefFormat(buf, def->onReboot, "on_reboot", virDomainLifecycleTypeToString) < 0) goto cleanup; - if (virDomainLifecycleDefFormat(&buf, def->onCrash, + if (virDomainLifecycleDefFormat(buf, def->onCrash, "on_crash", virDomainLifecycleCrashTypeToString) < 0) goto cleanup; - virBufferAddLit(&buf, " <devices>\n"); + virBufferAddLit(buf, " <devices>\n"); if (def->emulator) - virBufferEscapeString(&buf, " <emulator>%s</emulator>\n", + virBufferEscapeString(buf, " <emulator>%s</emulator>\n", def->emulator); for (n = 0 ; n < def->ndisks ; n++) - if (virDomainDiskDefFormat(&buf, def->disks[n], flags) < 0) + if (virDomainDiskDefFormat(buf, def->disks[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->ncontrollers ; n++) - if (virDomainControllerDefFormat(&buf, def->controllers[n], flags) < 0) + if (virDomainControllerDefFormat(buf, def->controllers[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nleases ; n++) - if (virDomainLeaseDefFormat(&buf, def->leases[n]) < 0) + if (virDomainLeaseDefFormat(buf, def->leases[n]) < 0) goto cleanup; for (n = 0 ; n < def->nfss ; n++) - if (virDomainFSDefFormat(&buf, def->fss[n], flags) < 0) + if (virDomainFSDefFormat(buf, def->fss[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nnets ; n++) - if (virDomainNetDefFormat(&buf, def->nets[n], flags) < 0) + if (virDomainNetDefFormat(buf, def->nets[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nsmartcards ; n++) - if (virDomainSmartcardDefFormat(&buf, def->smartcards[n], flags) < 0) + if (virDomainSmartcardDefFormat(buf, def->smartcards[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nserials ; n++) - if (virDomainChrDefFormat(&buf, def->serials[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->serials[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nparallels ; n++) - if (virDomainChrDefFormat(&buf, def->parallels[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->parallels[n], flags) < 0) goto cleanup; /* If there's a PV console that's preferred.. */ if (def->console) { - if (virDomainChrDefFormat(&buf, def->console, flags) < 0) + if (virDomainChrDefFormat(buf, def->console, flags) < 0) goto cleanup; } else if (def->nserials != 0) { /* ..else for legacy compat duplicate the first serial device as a @@ -10256,17 +10259,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virDomainChrDef console; memcpy(&console, def->serials[0], sizeof(console)); console.deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE; - if (virDomainChrDefFormat(&buf, &console, flags) < 0) + if (virDomainChrDefFormat(buf, &console, flags) < 0) goto cleanup; } for (n = 0 ; n < def->nchannels ; n++) - if (virDomainChrDefFormat(&buf, def->channels[n], flags) < 0) + if (virDomainChrDefFormat(buf, def->channels[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->ninputs ; n++) if (def->inputs[n]->bus == VIR_DOMAIN_INPUT_BUS_USB && - virDomainInputDefFormat(&buf, def->inputs[n], flags) < 0) + virDomainInputDefFormat(buf, def->inputs[n], flags) < 0) goto cleanup; if (def->ngraphics > 0) { @@ -10278,33 +10281,33 @@ virDomainDefFormatInternal(virDomainDefPtr def, { .alias = NULL }, }; - if (virDomainInputDefFormat(&buf, &autoInput, flags) < 0) + if (virDomainInputDefFormat(buf, &autoInput, flags) < 0) goto cleanup; for (n = 0 ; n < def->ngraphics ; n++) - if (virDomainGraphicsDefFormat(&buf, def->graphics[n], flags) < 0) + if (virDomainGraphicsDefFormat(buf, def->graphics[n], flags) < 0) goto cleanup; } for (n = 0 ; n < def->nsounds ; n++) - if (virDomainSoundDefFormat(&buf, def->sounds[n], flags) < 0) + if (virDomainSoundDefFormat(buf, def->sounds[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nvideos ; n++) - if (virDomainVideoDefFormat(&buf, def->videos[n], flags) < 0) + if (virDomainVideoDefFormat(buf, def->videos[n], flags) < 0) goto cleanup; for (n = 0 ; n < def->nhostdevs ; n++) - if (virDomainHostdevDefFormat(&buf, def->hostdevs[n], flags) < 0) + if (virDomainHostdevDefFormat(buf, def->hostdevs[n], flags) < 0) goto cleanup; if (def->watchdog) - virDomainWatchdogDefFormat (&buf, def->watchdog, flags); + virDomainWatchdogDefFormat (buf, def->watchdog, flags); if (def->memballoon) - virDomainMemballoonDefFormat (&buf, def->memballoon, flags); + virDomainMemballoonDefFormat (buf, def->memballoon, flags); - virBufferAddLit(&buf, " </devices>\n"); + virBufferAddLit(buf, " </devices>\n"); if (def->seclabel.model) { const char *sectype = virDomainSeclabelTypeToString(def->seclabel.type); @@ -10316,47 +10319,52 @@ virDomainDefFormatInternal(virDomainDefPtr def, (flags & VIR_DOMAIN_XML_INACTIVE)) { /* This is the default for inactive xml, so nothing to output. */ } else { - virBufferAsprintf(&buf, " <seclabel type='%s' model='%s' relabel='%s'>\n", + virBufferAsprintf(buf, " <seclabel type='%s' model='%s' relabel='%s'>\n", sectype, def->seclabel.model, def->seclabel.norelabel ? "no" : "yes"); if (def->seclabel.label) - virBufferEscapeString(&buf, " <label>%s</label>\n", + virBufferEscapeString(buf, " <label>%s</label>\n", def->seclabel.label); if (!def->seclabel.norelabel && def->seclabel.imagelabel) - virBufferEscapeString(&buf, " <imagelabel>%s</imagelabel>\n", + virBufferEscapeString(buf, " <imagelabel>%s</imagelabel>\n", def->seclabel.imagelabel); if (def->seclabel.baselabel && (def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC)) - virBufferEscapeString(&buf, " <baselabel>%s</baselabel>\n", + virBufferEscapeString(buf, " <baselabel>%s</baselabel>\n", def->seclabel.baselabel); - virBufferAddLit(&buf, " </seclabel>\n"); + virBufferAddLit(buf, " </seclabel>\n"); } } if (def->namespaceData && def->ns.format) { - if ((def->ns.format)(&buf, def->namespaceData) < 0) + if ((def->ns.format)(buf, def->namespaceData) < 0) goto cleanup; } - virBufferAddLit(&buf, "</domain>\n"); + virBufferAddLit(buf, "</domain>\n"); - if (virBufferError(&buf)) + if (virBufferError(buf)) goto no_memory; - return virBufferContentAndReset(&buf); + return 0; no_memory: virReportOOMError(); cleanup: - virBufferFreeAndReset(&buf); - return NULL; + virBufferFreeAndReset(buf); + return -1; } char * virDomainDefFormat(virDomainDefPtr def, unsigned int flags) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + virCheckFlags(DUMPXML_FLAGS, NULL); - return virDomainDefFormatInternal(def, flags); + if (virDomainDefFormatInternal(def, flags, &buf) < 0) + return NULL; + + return virBufferContentAndReset(&buf); } @@ -10364,7 +10372,6 @@ static char *virDomainObjFormat(virCapsPtr caps, virDomainObjPtr obj, unsigned int flags) { - char *config_xml = NULL; virBuffer buf = VIR_BUFFER_INITIALIZER; int state; int reason; @@ -10386,11 +10393,9 @@ static char *virDomainObjFormat(virCapsPtr caps, ((caps->privateDataXMLFormat)(&buf, obj->privateData)) < 0) goto error; - if (!(config_xml = virDomainDefFormatInternal(obj->def, flags))) + if (virDomainDefFormatInternal(obj->def, flags, &buf) < 0) goto error; - virBufferAdd(&buf, config_xml, strlen(config_xml)); - VIR_FREE(config_xml); virBufferAddLit(&buf, "</domstatus>\n"); if (virBufferError(&buf)) -- 1.7.4.4

* docs/schemas/domain.rng: Move guts... * docs/schemas/domaincommon.rng: ...to new file. * docs/schemas/domainsnapshot.rng: Allow new xml. * docs/schemas/Makefile.am (schema_DATA): Distribute new file. * tests/domainsnapshotxml2xmlout/full_domain.xml: New test. --- Email was post-processed (again). docs/schemas/Makefile.am | 1 + docs/schemas/domain.rng | 2555 +----------------------- docs/schemas/{domain.rng => domaincommon.rng} | 8 +- docs/schemas/domainsnapshot.rng | 14 +- tests/domainsnapshotxml2xmlout/full_domain.xml | 35 + 5 files changed, 49 insertions(+), 2564 deletions(-) copy docs/schemas/{domain.rng => domaincommon.rng} (99%) create mode 100644 tests/domainsnapshotxml2xmlout/full_domain.xml diff --git a/docs/schemas/Makefile.am b/docs/schemas/Makefile.am index 596c207..4413d9e 100644 --- a/docs/schemas/Makefile.am +++ b/docs/schemas/Makefile.am @@ -6,6 +6,7 @@ schema_DATA = \ basictypes.rng \ capability.rng \ domain.rng \ + domaincommon.rng \ domainsnapshot.rng \ interface.rng \ network.rng \ diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index dd8c41a..cf0be68 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -5,2 +5,2 @@ <ref name="domain"/> </start> - <include href='basictypes.rng'/> - <include href='storageencryption.rng'/> - <include href='networkcommon.rng'/> - - <!-- - description element, maybe placed anywhere under the root ... - <param name="pattern">[a-zA-Z0-9_\.:]+</param> - </data> - </define> + <include href='domaincommon.rng'/> </grammar> diff --git a/docs/schemas/domain.rng b/docs/schemas/domaincommon.rng similarity index 99% copy from docs/schemas/domain.rng copy to docs/schemas/domaincommon.rng index dd8c41a..756e892 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domaincommon.rng @@ -1,16 +1,12 @@ <?xml version="1.0"?> <grammar xmlns="http://relaxng.org/ns/structure/1.0" datatypeLibrary="http://www.w3.org/2001/XMLSchema-datatypes"> - <!-- We handle only document defining a domain --> - <start> - <ref name="domain"/> - </start> - + <!-- domain-related definitions used in multiple grammars --> <include href='basictypes.rng'/> <include href='storageencryption.rng'/> <include href='networkcommon.rng'/> <!-- - description element, maybe placed anywhere under the root + description element, may be placed anywhere under the root --> <define name="description"> <element name="description"> diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng index 410833f..a16d731 100644 --- a/docs/schemas/domainsnapshot.rng +++ b/docs/schemas/domainsnapshot.rng @@ -1,9 +1,12 @@ +<?xml version="1.0"?> <!-- A Relax NG schema for the libvirt domain snapshot properties XML format --> <grammar xmlns="http://relaxng.org/ns/structure/1.0"> <start> <ref name='domainsnapshot'/> </start> + <include href='domaincommon.rng'/> + <define name='domainsnapshot'> <element name='domainsnapshot'> <interleave> @@ -36,11 +39,14 @@ </element> </optional> <optional> - <element name='domain'> - <element name='uuid'> - <text/> + <choice> + <element name='domain'> + <element name='uuid'> + <ref name="UUID"/> + </element> </element> - </element> + <ref name='domain'/> + </choice> </optional> <optional> <element name='parent'> diff --git a/tests/domainsnapshotxml2xmlout/full_domain.xml b/tests/domainsnapshotxml2xmlout/full_domain.xml new file mode 100644 index 0000000..942bd7f --- /dev/null +++ b/tests/domainsnapshotxml2xmlout/full_domain.xml @@ -0,0 +1,35 @@ +<domainsnapshot> + <name>my snap name</name> + <description>!@#$%^</description> + <parent> + <name>earlier_snap</name> + </parent> + <state>running</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> -- 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 44a553b..24b34b6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1504,6 +1504,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 1a8a541..5e278ae 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

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 2fa25ea..754c619 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -12757,6 +12757,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} }; @@ -12781,13 +12784,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 e5d6b71..28e1d20 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1804,12 +1804,20 @@ I<--running> or I<--paused> flag will perform additional state changes transient domains cannot be inactive, it is required to use one of these flags when reverting to a disk snapshot of a transient domain. -=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

Easy enough to emulate even with older servers. * tools/virsh.c (cmdSnapshotCreate, cmdSnapshotCreateAs): Add --halt flag. (vshSnapshotCreate): Emulate halt when flag is unsupported. * tools/virsh.pod (snapshot-create, snapshot-create-as): Document it. --- tools/virsh.c | 41 +++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 11 ++++++++--- 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 754c619..15e30e9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11986,15 +11986,45 @@ vshSnapshotCreate(vshControl *ctl, virDomainPtr dom, const char *buffer, { bool ret = false; virDomainSnapshotPtr snapshot; + bool halt = false; char *doc = NULL; xmlDocPtr xml = NULL; xmlXPathContextPtr ctxt = NULL; char *name = NULL; snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + + /* Emulate --halt on older servers. */ + if (!snapshot && last_error->code == VIR_ERR_INVALID_ARG && + (flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT)) { + int persistent; + + virFreeError(last_error); + last_error = NULL; + persistent = virDomainIsPersistent(dom); + if (persistent < 0) { + virshReportError(ctl); + goto cleanup; + } + if (!persistent) { + vshError(ctl, "%s", + _("cannot halt after snapshot of transient domain")); + goto cleanup; + } + if (virDomainIsActive(dom) == 1) + halt = true; + flags &= ~VIR_DOMAIN_SNAPSHOT_CREATE_HALT; + snapshot = virDomainSnapshotCreateXML(dom, buffer, flags); + } + if (snapshot == NULL) goto cleanup; + if (halt && virDomainDestroy(dom) < 0) { + virshReportError(ctl); + goto cleanup; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA) doc = vshStrdup(ctl, buffer); else @@ -12045,6 +12075,7 @@ static const vshCmdOptDef opts_snapshot_create[] = { {"redefine", VSH_OT_BOOL, 0, N_("redefine metadata for existing snapshot")}, {"current", VSH_OT_BOOL, 0, N_("with redefine, set current snapshot")}, {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, + {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, {NULL, 0, 0, NULL} }; @@ -12063,6 +12094,8 @@ cmdSnapshotCreate(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT; if (vshCommandOptBool(cmd, "no-metadata")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; + if (vshCommandOptBool(cmd, "halt")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12113,6 +12146,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")}, {"no-metadata", VSH_OT_BOOL, 0, N_("take snapshot but create no metadata")}, + {"halt", VSH_OT_BOOL, 0, N_("halt domain after snapshot is created")}, {NULL, 0, 0, NULL} }; @@ -12129,6 +12163,8 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "no-metadata")) flags |= VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA; + if (vshCommandOptBool(cmd, "halt")) + flags |= VIR_DOMAIN_SNAPSHOT_CREATE_HALT; if (!vshConnectionUsability(ctl, ctl->conn)) goto cleanup; @@ -12157,6 +12193,11 @@ cmdSnapshotCreateAs(vshControl *ctl, const vshCmd *cmd) } if (vshCommandOptBool(cmd, "print-xml")) { + if (vshCommandOptBool(cmd, "halt")) { + vshError(ctl, "%s", + _("--print-xml and --halt are mutually exclusive")); + goto cleanup; + } vshPrint(ctl, "%s\n", buffer); ret = true; goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 28e1d20..30548b4 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1692,7 +1692,7 @@ used to represent properties of snapshots. =over 4 =item B<snapshot-create> I<domain> [I<xmlfile>] {[I<--redefine> [I<--current>]] -| [I<--no-metadata>]} +| [I<--no-metadata>] [I<--halt>]} Create a snapshot for domain I<domain> with the properties specified in I<xmlfile>. Normally, the only properties settable for a domain snapshot @@ -1701,6 +1701,9 @@ ignored, and automatically filled in by libvirt. If I<xmlfile> is completely omitted, then libvirt will choose a value for all fields. The new snapshot will become current, as listed by B<snapshot-current>. +If I<--halt> is specified, the domain will be left in an inactive state +after the snapshot is created. + If I<--redefine> is specified, then all XML elements produced by B<snapshot-dumpxml> are valid; this can be used to migrate snapshot hierarchy from one machine to another, to recreate hierarchy for the @@ -1722,13 +1725,15 @@ a persistent domain. However, for transient domains, snapshot metadata is silently lost when the domain quits running (whether by command such as B<destroy> or by internal guest action). -=item B<snapshot-create-as> I<domain> {[I<--print-xml>] | [I<--no-metadata>]} -[I<name>] [I<description>] +=item B<snapshot-create-as> I<domain> {[I<--print-xml>] +| [I<--no-metadata>] [I<--halt>]} [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. If I<--no-metadata> is specified, then the snapshot data is created, but any metadata is immediately discarded (that is, libvirt does not -- 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. (VIR_DOMAIN_DISK_SNAPSHOT): New internal enum value. * 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. * 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 | 76 ++++++++++++++++++++-- docs/schemas/domainsnapshot.rng | 15 ++++- include/libvirt/libvirt.h.in | 14 ++++- src/conf/domain_conf.c | 26 ++++++-- src/conf/domain_conf.h | 8 ++- src/libvirt_private.syms | 2 + tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 35 ++++++++++ tools/virsh.c | 6 ++ 8 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 tests/domainsnapshotxml2xmlout/disk_snapshot.xml diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 1e2d28a..99ff4d4 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, normally only the <code>name</code> @@ -53,14 +108,21 @@ </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 default to whatever is in this field. Readonly. + 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" + (<span class="since">since 0.9.5</span>) 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, as described above. 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 @@ -70,7 +132,7 @@ created in, and requires the use of the <code>VIR_DOMAIN_SNAPSHOT_REVERT_FORCE</code> flag in <code>virDomainRevertToSnapshot()</code>. Newer versions - of libvirt store the entire + of libvirt (<span class="since">since 0.9.5</span>) store the entire inactive <a href="formatdomain.html">domain configuration</a> at the time of the snapshot. Readonly. </dd> 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 b35042c..f1b2bf3 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 { @@ -1890,6 +1897,11 @@ typedef enum { VIR_KEYCODE_SET_WIN32 = 8, VIR_KEYCODE_SET_RFB = 9, + /* + * 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 36a8252..6e9d822 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_DISK_SNAPSHOT+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") @@ -11064,7 +11075,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"), @@ -11137,7 +11148,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid, virBufferEscapeString(&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"); virBufferEscapeString(&buf, " <name>%s</name>\n", def->parent); @@ -11717,6 +11728,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) { @@ -11750,9 +11762,9 @@ virDomainStateReasonToString(virDomainState state, int reason) return virDomainShutoffReasonTypeToString(reason); case VIR_DOMAIN_CRASHED: return virDomainCrashedReasonTypeToString(reason); + default: + return NULL; } - - return NULL; } @@ -11774,9 +11786,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 1be7ed9..c92fc4b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -226,6 +226,11 @@ enum virDomainDiskSnapshot { VIR_DOMAIN_DISK_SNAPSHOT_LAST }; +enum virDomainSnapshotState { + /* Inherit the VIR_DOMAIN_* states from virDomainState. */ + VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST, +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -1312,7 +1317,7 @@ struct _virDomainSnapshotDef { char *description; char *parent; long long creationTime; /* in seconds */ - int state; + int state; /* enum virDomainSnapshotState */ virDomainDefPtr dom; /* Internal use. */ @@ -1743,6 +1748,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 9f6c784..03819a4 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 15e30e9..14ec429 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -14576,6 +14576,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"); @@ -14590,6 +14592,7 @@ vshDomainStateToString(int state) case VIR_DOMAIN_CRASHED: return N_("crashed"); case VIR_DOMAIN_NOSTATE: + default: ;/*FALLTHROUGH*/ } return N_("no state"); /* = dom0 state */ @@ -14691,6 +14694,9 @@ vshDomainStateReasonToString(int state, int reason) ; } break; + + default: + ; } return N_("unknown"); -- 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 | 102 ++++++++++------------ 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, 154 insertions(+), 129 deletions(-) diff --git a/docs/formatsnapshot.html.in b/docs/formatsnapshot.html.in index 9c7631c..ffc626b 100644 --- a/docs/formatsnapshot.html.in +++ b/docs/formatsnapshot.html.in @@ -125,8 +125,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 @@ -198,7 +199,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 aa0ad83..f9ca99c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5518,17 +5518,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, @@ -5604,7 +5631,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); @@ -11229,11 +11256,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, @@ -11271,7 +11299,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) { @@ -11309,6 +11337,13 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, disk->file, disk->name); goto cleanup; } + if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) { + VIR_FREE(disk->name); + if (!(disk->name = 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 2e34ace..f45711b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1562,7 +1562,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 1aa33ef..8c036ce 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 91da438..4302c8b 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 d3968cf..c7e195f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5443,7 +5443,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; @@ -5561,10 +5561,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]; @@ -7303,7 +7303,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); @@ -7325,42 +7326,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) @@ -7475,8 +7469,8 @@ static int qemuDomainGetBlockInfo(virDomainPtr dom, virStorageFileMetadata *meta = NULL; virDomainDiskDefPtr disk = NULL; struct stat sb; - int i; int format; + const char *actual; virCheckFlags(0, -1); @@ -7498,19 +7492,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); @@ -9782,23 +9769,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 c22974f..f36efea 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 0a7fb48..8e21701 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -3849,11 +3849,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; @@ -3889,18 +3889,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 30548b4..c40e9f3 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -503,7 +503,9 @@ snapshot metadata with B<snapshot-create>. =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> @@ -515,7 +517,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 | 6 ++++++ src/vbox/vbox_tmpl.c | 6 ++++++ 5 files changed, 26 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f1b2bf3..0eee6d6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2579,6 +2579,8 @@ typedef enum { remembering it */ VIR_DOMAIN_SNAPSHOT_CREATE_HALT = (1 << 3), /* Stop running guest after snapshot */ + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY = (1 << 4), /* 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 1458828..ae189bd 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -4226,6 +4226,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 0ae6cc2..3cdff36 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -15638,6 +15638,12 @@ error: * running after the snapshot. This flag is invalid on transient domains, * and is incompatible with VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE. * + * 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 c7e195f..19897c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8816,6 +8816,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (!qemuDomainSnapshotIsAllowed(vm)) 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 cbe34e8..0e4364f 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -5661,6 +5661,12 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom, if (!(def = virDomainSnapshotDefParseString(xmlDesc, 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. (qemuDomainUndefineFlags, qemuDomainSnapshotDelete): Use it to add safety valve. (qemuDomainRevertToSnapshot): Add safety valve. --- src/qemu/qemu_driver.c | 36 ++++++++++++++++++++++++++++++++++++ 1 files changed, 36 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 19897c6..d4e9452 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1782,6 +1782,19 @@ qemuDomainSnapshotDiscardAll(void *payload, curr->err = err; } +/* 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 qemuDomainDestroyFlags(virDomainPtr dom, unsigned int flags) @@ -9193,6 +9206,12 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, "to revert to inactive snapshot")); goto cleanup; } + if (snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("revert to external disk snapshot not supported " + "yet")); + goto cleanup; + } if (vm->current_snapshot) { vm->current_snapshot->def->current = false; @@ -9479,6 +9498,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 | @@ -9501,6 +9521,22 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, goto cleanup; } + if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY)) { + if (!(flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) && + snap->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + external++; + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) + virDomainSnapshotForEachDescendant(&vm->snapshots, snap, + qemuDomainSnapshotCountExternal, + &external); + if (external) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("deletion of %d external disk snapshots not " + "supported yet"), external); + goto cleanup; + } + } + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; -- 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. --- src/qemu/qemu_driver.c | 290 +++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 272 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d4e9452..4862c95 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -298,6 +298,7 @@ static void qemuDomainSnapshotLoad(void *payload, virDomainSnapshotObjPtr current = NULL; char ebuf[1024]; unsigned int flags = (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS | VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); virDomainObjLock(vm); @@ -8748,6 +8749,241 @@ 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) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("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, @@ -8766,7 +9002,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE | VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT | VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA | - VIR_DOMAIN_SNAPSHOT_CREATE_HALT, NULL); + VIR_DOMAIN_SNAPSHOT_CREATE_HALT | + VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, NULL); if (((flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) || @@ -8774,6 +9011,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, update_current = false; if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE) parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE; + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) + parse_flags |= VIR_DOMAIN_SNAPSHOT_PARSE_DISKS; qemuDriverLock(driver); virUUIDFormat(domain->uuid, uuidstr); @@ -8817,22 +9056,29 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, QEMU_EXPECTED_VIRT_TYPES, 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 (def->ndisks) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("disk snapshots not supported yet")); - 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; + def->state = virDomainObjGetState(vm, NULL); + } } if (!(snap = virDomainSnapshotAssignDef(&vm->snapshots, def))) @@ -8841,8 +9087,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, if (update_current) snap->def->current = true; - if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) - snap->def->state = virDomainObjGetState(vm, NULL); if (vm->current_snapshot) { if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) { snap->def->parent = strdup(vm->current_snapshot->def->name); @@ -8867,6 +9111,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, * snapshot exists and is not creating a loop, or that * qemu-img recognizes the snapshot name in at least one of * the domain's disks? */ + } else 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; -- 1.7.4.4