virsh snapshot-create-as supports 'file' storage type in --diskspec by default.
It doesn't support 'block' storage type in the process of 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.

Signed-off-by: Liu Dayu <liu.dayu@zte.com.cn>
---
 tools/virsh-snapshot.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index f6bb38b..f9c55e0 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -255,6 +255,8 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
     char **array = NULL;
     int narray;
     size_t i;
+    struct stat st;
+    int stor_type = VIR_STORAGE_TYPE_NONE;
 
     narray = vshStringToArray(str, &array);
     if (narray <= 0)
@@ -272,16 +274,29 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
             goto cleanup;
     }
 
+    /* possibly update storage type */
+    if (file && STRPREFIX(file, "/dev/") && stat(file, &st) == 0 && S_ISBLK(st.st_mode))
+        stor_type = VIR_STORAGE_TYPE_BLOCK;
+    else
+        stor_type = VIR_STORAGE_TYPE_FILE;
+
     virBufferEscapeString(buf, "<disk name='%s'", name);
     if (snapshot)
         virBufferAsprintf(buf, " snapshot='%s'", snapshot);
+    if (stor_type == VIR_STORAGE_TYPE_BLOCK)
+        virBufferAsprintf(buf, " type='%s'", virStorageTypeToString(stor_type));
+
     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 (stor_type == VIR_STORAGE_TYPE_BLOCK)
+                virBufferEscapeString(buf, "<source dev='%s'/>\n", file);
+            else
+                virBufferEscapeString(buf, "<source file='%s'/>\n", file);
+        }
         virBufferAdjustIndent(buf, -2);
         virBufferAddLit(buf, "</disk>\n");
     } else {
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Jun 26, 2019 at 14:05:41 +0800, Liu Dayu wrote:
> virsh snapshot-create-as supports 'file' storage type in --diskspec by default.
> It doesn't support 'block' storage type in the process of 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.
> 
> Signed-off-by: Liu Dayu <liu.dayu@zte.com.cn>
> ---
>  tools/virsh-snapshot.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
> index f6bb38b..f9c55e0 100644
> --- a/tools/virsh-snapshot.c
> +++ b/tools/virsh-snapshot.c
> @@ -255,6 +255,8 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
>      char **array = NULL;
>      int narray;
>      size_t i;
> +    struct stat st;
> +    int stor_type = VIR_STORAGE_TYPE_NONE;
>  
>      narray = vshStringToArray(str, &array);
>      if (narray <= 0)
> @@ -272,16 +274,29 @@ virshParseSnapshotDiskspec(vshControl *ctl, virBufferPtr buf, const char *str)
>              goto cleanup;
>      }
>  
> +    /* possibly update storage type */
> +    if (file && STRPREFIX(file, "/dev/") && stat(file, &st) == 0 && S_ISBLK(st.st_mode))

virsh has to be able to work remotely so this code will definitely not
be acceptable here.

I think we can only go with adding a "type" field for the diskspec
string which will be either "file" or "block" and do this in that case.

e.g.

--diskspec vda,snapshot=external,driver=qcow2,type=block,file=/dev/whatever
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

> virsh has to be able to work remotely so this code will definitely not
> be acceptable here.
>
> I think we can only go with adding a "type" field for the diskspec
> string which will be either "file" or "block" and do this in that case.
> e.g.
> --diskspec vda,snapshot=external,driver=qcow2,type=block,file=/dev/whatever

thanks, i will reconsider adding a "type" field for --diskspec to amend this patch.--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list