snapshot: add REVERT_FORCE to API

Although reverting to a snapshot is a form of data loss, this is
normally expected.  However, there are two cases where additional
surprises (failure to run the reverted state, or a break in
connectivity to the domain) can come into play.  Requiring extra
acknowledgment in these cases will make it less likely that
someone can get into an unrecoverable state due to a default revert.

Also create a new error code, so users can distinguish when forcing
would make a difference, rather than having to blindly request force.

* include/libvirt/libvirt.h.in (VIR_DOMAIN_SNAPSHOT_REVERT_FORCE):
New flag.
* src/libvirt.c (virDomainRevertToSnapshot): Document it.
* include/libvirt/virterror.h (VIR_ERR_SNAPSHOT_REVERT_RISKY): New
error value.
* src/util/virterror.c (virErrorMsg): Implement it.
* tools/virsh.c (cmdDomainSnapshotRevert): Add --force to virsh.
* tools/virsh.pod (snapshot-revert): Document it.

include/libvirt/libvirt.h.in

@@ -2723,6 +2723,7 @@ virDomainSnapshotPtr virDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
 typedef enum {
     VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
     VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED  = 1 << 1, /* Pause after revert */
+    VIR_DOMAIN_SNAPSHOT_REVERT_FORCE   = 1 << 2, /* Allow risky reverts */
 } virDomainSnapshotRevertFlags;
 
 /* Revert the domain to a point-in-time snapshot.  The

include/libvirt/virterror.h

@@ -237,6 +237,8 @@ typedef enum {
                                            the given driver */
     VIR_ERR_STORAGE_PROBE_FAILED = 75,  /* storage pool proble failed */
     VIR_ERR_STORAGE_POOL_BUILT = 76,    /* storage pool already built */
+    VIR_ERR_SNAPSHOT_REVERT_RISKY = 77, /* force was not requested for a
+                                           risky domain snapshot revert */
 } virErrorNumber;
 
 /**

src/libvirt.c

@@ -16262,6 +16262,28 @@ error:
  * into an inactive state, so transient domains require the use of one
  * of these two flags.
  *
+ * Reverting to any snapshot discards all configuration changes made since
+ * the last snapshot.  Additionally, reverting to a snapshot from a running
+ * domain is a form of data loss, since it discards whatever is in the
+ * guest's RAM at the time.  Since the very nature of keeping snapshots
+ * implies the intent to roll back state, no additional confirmation is
+ * normally required for these lossy effects.
+ *
+ * However, there are two particular situations where reverting will
+ * be refused by default, and where @flags must include
+ * VIR_DOMAIN_SNAPSHOT_REVERT_FORCE to acknowledge the risks.  1) Any
+ * attempt to revert to a snapshot that lacks the metadata to perform
+ * ABI compatibility checks (generally the case for snapshots that
+ * lack a full <domain> when listed by virDomainSnapshotGetXMLDesc(),
+ * such as those created prior to libvirt 0.9.5).  2) Any attempt to
+ * revert a running domain to an active state that requires starting a
+ * new hypervisor instance rather than reusing the existing hypervisor
+ * (since this would terminate all connections to the domain, such as
+ * such as VNC or Spice graphics) - this condition arises from active
+ * snapshots that are provably ABI incomaptible, as well as from
+ * inactive snapshots with a @flags request to start the domain after
+ * the revert.
+ *
  * Returns 0 if the creation is successful, -1 on error.
  */
 int

src/util/virterror.c

@@ -1204,6 +1204,12 @@ virErrorMsg(virErrorNumber error, const char *info)
             else
                 errmsg = _("argument unsupported: %s");
             break;
+        case VIR_ERR_SNAPSHOT_REVERT_RISKY:
+            if (info == NULL)
+                errmsg = _("revert requires force");
+            else
+                errmsg = _("revert requires force: %s");
+            break;
     }
     return (errmsg);
 }

tools/virsh.c

@@ -13446,6 +13446,7 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
     {"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")},
+    {"force", VSH_OT_BOOL, 0, N_("try harder on risky reverts")},
     {NULL, 0, 0, NULL}
 };
 
@@ -13457,11 +13458,19 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
     const char *name = NULL;
     virDomainSnapshotPtr snapshot = NULL;
     unsigned int flags = 0;
+    bool force = false;
+    int result;
 
     if (vshCommandOptBool(cmd, "running"))
         flags |= VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING;
     if (vshCommandOptBool(cmd, "paused"))
         flags |= VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED;
+    /* We want virsh snapshot-revert --force to work even when talking
+     * to older servers that did the unsafe revert by default but
+     * reject the flag, so we probe without the flag, and only use it
+     * when the error says it will make a difference.  */
+    if (vshCommandOptBool(cmd, "force"))
+        force = true;
 
     if (!vshConnectionUsability(ctl, ctl->conn))
         goto cleanup;
@@ -13477,7 +13486,15 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
     if (snapshot == NULL)
         goto cleanup;
 
-    if (virDomainRevertToSnapshot(snapshot, flags) < 0)
+    result = virDomainRevertToSnapshot(snapshot, flags);
+    if (result < 0 && force &&
+        last_error->code == VIR_ERR_SNAPSHOT_REVERT_RISKY) {
+        flags |= VIR_DOMAIN_SNAPSHOT_REVERT_FORCE;
+        virFreeError(last_error);
+        last_error = NULL;
+        result = virDomainRevertToSnapshot(snapshot, flags);
+    }
+    if (result < 0)
         goto cleanup;
 
     ret = true;

tools/virsh.pod

@@ -1995,6 +1995,7 @@ Using I<--security-info> will also include security sensitive information.
 Output the name of the parent snapshot for the given I<snapshot>, if any.
 
 =item B<snapshot-revert> I<domain> I<snapshot> [{I<--running> | I<--paused>}]
+[I<--force>]
 
 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 last
@@ -2010,6 +2011,22 @@ 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.
 
+There are two cases where a snapshot revert involves extra risk, which
+requires the use of I<--force> to proceed.  One is the case of a
+snapshot that lacks full domain information for reverting
+configuration (such as snapshots created prior to libvirt 0.9.5);
+since libvirt cannot prove that the current configuration matches what
+was in use at the time of the snapshot, supplying I<--force> assures
+libvirt that the snapshot is compatible with the current configuration
+(and if it is not, the domain will likely fail to run).  The other is
+the case of reverting from a running domain to an active state where a
+new hypervisor has to be created rather than reusing the existing
+hypervisor, because it implies drawbacks such as breaking any existing
+VNC or Spice connections; this condition happens with an active
+snapshot that uses a provably incompatible configuration, as well as
+with an inactive snapshot that is combined with the I<--start> or
+I<--pause> flag.
+
 =item B<snapshot-delete> I<domain> I<snapshot> [I<--metadata>]
 [{I<--children> | I<--children-only>}]