[libvirt] [PATCH v2] libvirt: support block device storage type in virshParseSnapshotDiskspec

Liu Dayu posted 1 patch 4 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1562579194-15765-1-git-send-email-liu.dayu@zte.com.cn
tools/virsh-snapshot.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
[libvirt] [PATCH v2] libvirt: support block device storage type in virshParseSnapshotDiskspec
Posted by Liu Dayu 4 years, 8 months ago
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>
---
Patch v1:
https://www.redhat.com/archives/libvir-list/2019-June/msg01248.html

Changes in v2:
 - Adding a "stype" field for the --diskspec string which will indicate a "file" or "block" storage type.
---
 tools/virsh-snapshot.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index e9f0ee0..ec965b2 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}
 };
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] libvirt: support block device storage type in virshParseSnapshotDiskspec
Posted by Peter Krempa 4 years, 8 months ago
On Mon, Jul 08, 2019 at 17:46:34 +0800, Liu Dayu wrote:
> 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>
> ---
> Patch v1:
> https://www.redhat.com/archives/libvir-list/2019-June/msg01248.html
> 
> Changes in v2:
>  - Adding a "stype" field for the --diskspec string which will indicate a "file" or "block" storage type.
> ---
>  tools/virsh-snapshot.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)

[...]

> @@ -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);
> +    } 

There is a (invisible) trailing whitespace. I'll fix it.

>      if (driver || file) {

[...]

> @@ -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]")
>      },

This patch should also add docs to tools/virsh.pod which is used to
generate the man page for virsh. I'll add some docs before pushing.

Thanks for addressing my comments I'll push this one shortly with the
aforementioned changes.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] libvirt: support block device storage type in virshParseSnapshotDiskspec
Posted by Eric Blake 4 years, 8 months ago
On 7/8/19 4:46 AM, Liu Dayu wrote:
> 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>
> ---
> Patch v1:
> https://www.redhat.com/archives/libvir-list/2019-June/msg01248.html
> 
> Changes in v2:
>  - Adding a "stype" field for the --diskspec string which will indicate a "file" or "block" storage type.

I'm a little late for bike-shedding, but could we have supported:
--diskspec vda,block=/dev/xxx
instead of the more verbose
--diskspec vda,file=/dev/xxx,stype=block


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt][PATCH v2] libvirt: support block device storage typein virshParseSnapshotDiskspec
Posted by liu.dayu@zte.com.cn 4 years, 8 months ago
>> --->> Patch v1:>> https://www.redhat.com/archives/libvir-list/2019-June/msg01248.html>> >> Changes in v2:>>  - Adding a "stype" field for the --diskspec string which will indicate a "file" or "block" storage type.>> I'm a little late for bike-shedding, but could we have supported:> --diskspec vda,block=/dev/xxx> instead of the more verbose> --diskspec vda,file=/dev/xxx,stype=block


This way is concise and more directly. Both OK currently.


But if --diskspec options extend in the future, 


e.g. --diskspec vda,stype=block,file=/dev/xxx,running=/dev/yyy,backing=/dev/zzz,...


maybe 'stype' is a unified way to indicate those storage types other than change prefix name.











原始邮件



发件人:EricBlake <eblake@redhat.com>
收件人:刘大宇10031743;libvir-list@redhat.com <libvir-list@redhat.com>;
抄送人:汪翼10129963;王业超10154425;
日 期 :2019年07月09日 21:48
主 题 :Re: [libvirt] [PATCH v2] libvirt: support block device storage typein virshParseSnapshotDiskspec


On 7/8/19 4:46 AM, Liu Dayu wrote:
> 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>
> ---
> Patch v1:
> https://www.redhat.com/archives/libvir-list/2019-June/msg01248.html
> 
> Changes in v2:
>  - Adding a "stype" field for the --diskspec string which will indicate a "file" or "block" storage type.

I'm a little late for bike-shedding, but could we have supported:
--diskspec vda,block=/dev/xxx
instead of the more verbose
--diskspec vda,file=/dev/xxx,stype=block


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list