From 7ca81e6f19d6e62400b422d231a9dfc428d6b59e Mon Sep 17 00:00:00 2001
From: Liu Dayu <liu.dayu@zte.com.cn>
Date: Mon, 8 Jul 2019 17:46:34 +0800
Subject: [PATCH] virsh: support block device storage type in
 virshParseSnapshotDiskspec

virsh snapshot-create-as supports 'file' storage type in --diskspec by default.
But it doesn't support 'block' storage type in the virshParseSnapshotDiskspec().
So if a snapshot on a block device (e.g. LV) was created, the type of
current running storage source in dumpxml is inconsistent with the actual
backend storage source. It will check file-system type mismatch failed
and return an error message of 'Migration without shared storage is unsafe'
when VM performs a live migration after this snapshot.

Considering virsh has to be able to work remotely that recognizing a block device
by prefix /dev/ or by stat() may be not suitable, so adding a "stype" field
for the --diskspec string which will be either "file" or "block".
e.g. --diskspec vda,snapshot=external,driver=qcow2,stype=block,file=/dev/xxx.

Signed-off-by: Liu Dayu <liu.dayu@zte.com.cn>
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tools/virsh-snapshot.c | 23 ++++++++++++++++++++---
 tools/virsh.pod        | 13 ++++++++-----
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index e9f0ee0810..be11a7491c 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -251,10 +251,12 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
     const char *name = NULL;
     const char *snapshot = NULL;
     const char *driver = NULL;
+    const char *stype = NULL;
     const char *file = NULL;
     char **array = NULL;
     int narray;
     size_t i;
+    bool isFile = true;
 
     narray = vshStringToArray(str, &array);
     if (narray <= 0)
@@ -266,6 +268,8 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
             snapshot = array[i] + strlen("snapshot=");
         else if (!driver && STRPREFIX(array[i], "driver="))
             driver = array[i] + strlen("driver=");
+        else if (!stype && STRPREFIX(array[i], "stype="))
+            stype = array[i] + strlen("stype=");
         else if (!file && STRPREFIX(array[i], "file="))
             file = array[i] + strlen("file=");
         else
@@ -275,13 +279,26 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
     virBufferEscapeString(buf, "<disk name='%s'", name);
     if (snapshot)
         virBufferAsprintf(buf, " snapshot='%s'", snapshot);
+    if (stype) {
+        if (STREQ(stype, "block")) {
+            isFile = false;
+        } else if (STRNEQ(stype, "file")) {
+            vshError(ctl, _("Unknown storage type: '%s'"), stype);
+            goto cleanup;
+        }
+        virBufferAsprintf(buf, " type='%s'", stype);
+    }
     if (driver || file) {
         virBufferAddLit(buf, ">\n");
         virBufferAdjustIndent(buf, 2);
         if (driver)
             virBufferAsprintf(buf, "<driver type='%s'/>\n", driver);
-        if (file)
-            virBufferEscapeString(buf, "<source file='%s'/>\n", file);
+        if (file) {
+            if (isFile)
+                virBufferEscapeString(buf, "<source file='%s'/>\n", file);
+            else
+                virBufferEscapeString(buf, "<source dev='%s'/>\n", file);
+        }
         virBufferAdjustIndent(buf, -2);
         virBufferAddLit(buf, "</disk>\n");
     } else {
@@ -351,7 +368,7 @@ static const vshCmdOptDef opts_snapshot_create_as[] = {
     },
     {.name = "diskspec",
      .type = VSH_OT_ARGV,
-     .help = N_("disk attributes: disk[,snapshot=type][,driver=type][,file=name]")
+     .help = N_("disk attributes: disk[,snapshot=type][,driver=type][,stype=type][,file=name]")
     },
     {.name = NULL}
 };
diff --git a/tools/virsh.pod b/tools/virsh.pod
index dc39004a66..5168fa96b6 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4676,11 +4676,14 @@ The I<--diskspec> option can be used to control how I<--disk-only> and
 external full system snapshots create external files.  This option can occur
 multiple times, according to the number of <disk> elements in the domain
 xml.  Each <diskspec> is in the
-form B<disk[,snapshot=type][,driver=type][,file=name]>.  A I<diskspec>
-must be provided for disks backed by block devices as libvirt doesn't
-auto-generate file names for those.  To include a
-literal comma in B<disk> or in B<file=name>, escape it with a second
-comma.  A literal I<--diskspec> must precede each B<diskspec> unless
+form B<disk[,snapshot=type][,driver=type][,stype=type][,file=name]>.
+A I<diskspec> must be provided for disks backed by block devices as libvirt
+doesn't auto-generate file names for those.  The optional B<stype> parameter
+allows to control the type of the source file. Supported values are 'file'
+(default) and 'block'.
+
+To include a literal comma in B<disk> or in B<file=name>, escape it with a
+second comma.  A literal I<--diskspec> must precede each B<diskspec> unless
 all three of I<domain>, I<name>, and I<description> are also present.
 For example, a diskspec of "vda,snapshot=external,file=/path/to,,new"
 results in the following XML: