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
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
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
>> --->> 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
© 2016 - 2024 Red Hat, Inc.