From: Ashish Mittal <ashish.mittal@veritas.com>
Sample XML for a VxHS disk:
<disk type='network' device='disk'>
<driver name='qemu' type='raw' cache='none'/>
<source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
<host name='192.168.0.1' port='9999'/>
</source>
<backingStore/>
<target dev='vda' bus='virtio'/>
<serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
<alias name='virtio-disk0'/>
<address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
</disk>
Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
---
v2 changelog:
(1) Added code for JSON parsing of a VxHS vdisk.
(2) Added test case to verify JSON parsing.
(3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
(4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
v3 changelog:
(1) Implemented the modern syntax for VxHS disk specification.
(2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
(3) Added a negative test case to check failure when multiple hosts
are specified for a VxHS disk.
v4 changelog:
(1) Fixes per review comments from v3.
(2) Had to remove a test from the previous version that checked for
error when multiple hosts are specified for VxHS device.
This started failing virschematest with the error
"XML document failed to validate against schema" as the
docs/schemas/domain.rng specifies only a single host.
docs/formatdomain.html.in | 15 ++++-
docs/schemas/domaincommon.rng | 13 ++++
src/libxl/libxl_conf.c | 1 +
src/qemu/qemu_command.c | 70 ++++++++++++++++++++++
src/qemu/qemu_driver.c | 3 +
src/qemu/qemu_parse_command.c | 25 ++++++++
src/util/virstoragefile.c | 64 +++++++++++++++++++-
src/util/virstoragefile.h | 1 +
src/xenconfig/xen_xl.c | 1 +
.../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24 ++++++++
tests/qemuargv2xmltest.c | 17 +++++-
.../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++
.../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++
tests/qemuxml2argvtest.c | 1 +
tests/virstoragetest.c | 19 ++++++
15 files changed, 308 insertions(+), 5 deletions(-)
create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 36bea67..62d67f4 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2501,9 +2501,9 @@
<dd>
The <code>protocol</code> attribute specifies the protocol to
access to the requested image. Possible values are "nbd",
- "iscsi", "rbd", "sheepdog" or "gluster". If the
- <code>protocol</code> attribute is "rbd", "sheepdog" or
- "gluster", an additional attribute <code>name</code> is
+ "iscsi", "rbd", "sheepdog", "gluster" or "vxhs". If the
+ <code>protocol</code> attribute is "rbd", "sheepdog", "gluster"
+ or "vxhs", an additional attribute <code>name</code> is
mandatory to specify which volume/image will be used. For "nbd",
the <code>name</code> attribute is optional. For "iscsi"
(<span class="since">since 1.0.4</span>), the <code>name</code>
@@ -2511,6 +2511,9 @@
target's name by a slash (e.g.,
<code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not
specified, the default LUN is zero.
+ For "vxhs" (<span class="since">since 3.3.0</span>), the
+ <code>name</code> is the UUID of the volume, assigned by the
+ HyperScale sever.
<span class="since">Since 0.8.7</span>
</dd>
<dt><code>volume</code></dt>
@@ -2613,6 +2616,12 @@
<td> one or more (<span class="since">Since 2.1.0</span>), just one prior to that </td>
<td> 24007 </td>
</tr>
+ <tr>
+ <td> vxhs </td>
+ <td> a server running Veritas HyperScale daemon </td>
+ <td> only one </td>
+ <td> 9999 </td>
+ </tr>
</table>
<p>
gluster supports "tcp", "rdma", "unix" as valid values for the
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index bdf7103..7525a2a 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1613,6 +1613,18 @@
</element>
</define>
+ <define name="diskSourceNetworkProtocolVxHS">
+ <element name="source">
+ <attribute name="protocol">
+ <choice>
+ <value>vxhs</value>
+ </choice>
+ </attribute>
+ <attribute name="name"/>
+ <ref name="diskSourceNetworkHost"/>
+ </element>
+ </define>
+
<define name="diskSourceNetwork">
<attribute name="type">
<value>network</value>
@@ -1623,6 +1635,7 @@
<ref name="diskSourceNetworkProtocolRBD"/>
<ref name="diskSourceNetworkProtocolHTTP"/>
<ref name="diskSourceNetworkProtocolSimple"/>
+ <ref name="diskSourceNetworkProtocolVxHS"/>
</choice>
</define>
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 938e09d..f12c796 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -665,6 +665,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
case VIR_STORAGE_NET_PROTOCOL_NONE:
virReportError(VIR_ERR_NO_SUPPORT,
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c53ab97..8e00782 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol,
return 0;
case VIR_STORAGE_NET_PROTOCOL_RBD:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
case VIR_STORAGE_NET_PROTOCOL_NONE:
/* not applicable */
@@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
}
+#define QEMU_DEFAULT_VXHS_PORT "9999"
+
+/* Build the VxHS host object */
+static virJSONValuePtr
+qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
+{
+ virJSONValuePtr server = NULL;
+ virStorageNetHostDefPtr host;
+ const char *portstr;
+
+ if (src->nhosts != 1) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("protocol VxHS accepts only one host"));
+ goto cleanup;
+ }
+
+ host = src->hosts;
+ portstr = host->port;
+
+ if (!portstr)
+ portstr = QEMU_DEFAULT_VXHS_PORT;
+
+ if (virJSONValueObjectCreate(&server,
+ "s:host", host->name,
+ "s:port", portstr,
+ NULL) < 0)
+ server = NULL;
+
+ cleanup:
+ return server;
+}
+
+
+static virJSONValuePtr
+qemuBuildVxHSDriveJSON(virStorageSourcePtr src)
+{
+ const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
+ virJSONValuePtr server = NULL;
+ virJSONValuePtr ret = NULL;
+
+ if (!(server = qemuBuildVxHSDriveJSONHost(src)))
+ return NULL;
+
+ /* VxHS disk specification example:
+ * { driver:"vxhs",
+ * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
+ * server.host:"1.2.3.4",
+ * server.port:1234}
+ */
+ if (virJSONValueObjectCreate(&ret,
+ "s:driver", protocol,
+ "s:vdisk-id", src->path,
+ "a:server", server, NULL) < 0)
+ virJSONValueFree(server);
+
+ return ret;
+}
+
+
static char *
qemuBuildNetworkDriveURI(virStorageSourcePtr src,
qemuDomainSecretInfoPtr secinfo)
@@ -1136,6 +1196,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
ret = virBufferContentAndReset(&buf);
break;
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("'VxHS' protocol does not support URI syntax"));
+ goto cleanup;
+
case VIR_STORAGE_NET_PROTOCOL_SSH:
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("'ssh' protocol is not yet supported"));
@@ -1180,6 +1245,11 @@ qemuGetDriveSourceProps(virStorageSourcePtr src,
if (!(fileprops = qemuBuildGlusterDriveJSON(src)))
return -1;
}
+
+ if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {
+ if (!(fileprops = qemuBuildVxHSDriveJSON(src)))
+ return -1;
+ }
break;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cdb727b..d43de69 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -13683,6 +13683,7 @@ qemuDomainSnapshotPrepareDiskExternalBackingInactive(virDomainDiskDefPtr disk)
case VIR_STORAGE_NET_PROTOCOL_FTPS:
case VIR_STORAGE_NET_PROTOCOL_TFTP:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("external inactive snapshots are not supported on "
@@ -13746,6 +13747,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
case VIR_STORAGE_NET_PROTOCOL_FTPS:
case VIR_STORAGE_NET_PROTOCOL_TFTP:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("external active snapshots are not supported on "
@@ -13891,6 +13893,7 @@ qemuDomainSnapshotPrepareDiskInternal(virConnectPtr conn,
case VIR_STORAGE_NET_PROTOCOL_FTPS:
case VIR_STORAGE_NET_PROTOCOL_TFTP:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("internal inactive snapshots are not supported on "
diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
index af9063c..aa15225 100644
--- a/src/qemu/qemu_parse_command.c
+++ b/src/qemu/qemu_parse_command.c
@@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk)
return -1;
}
+static int
+qemuParseVxHSString(virDomainDiskDefPtr def)
+{
+ virURIPtr uri = NULL;
+
+ if (!(uri = virURIParse(def->src->path)))
+ return -1;
+
+ return qemuParseDriveURIString(def, uri, "vxhs");
+}
+
/*
* This method takes a string representing a QEMU command line ARGV set
@@ -737,6 +748,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
if (VIR_STRDUP(def->src->path, vdi) < 0)
goto error;
}
+ } else if (STRPREFIX(def->src->path, "vxhs:")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("VxHS protocol does not support URI syntax '%s'"),
+ def->src->path);
+ goto error;
} else {
def->src->type = VIR_STORAGE_TYPE_FILE;
}
@@ -1945,6 +1961,10 @@ qemuParseCommandLine(virCapsPtr caps,
disk->src->type = VIR_STORAGE_TYPE_NETWORK;
disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_SHEEPDOG;
val += strlen("sheepdog:");
+ } else if (STRPREFIX(val, "vxhs:")) {
+ disk->src->type = VIR_STORAGE_TYPE_NETWORK;
+ disk->src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
+ val += strlen("vxhs:");
} else {
disk->src->type = VIR_STORAGE_TYPE_FILE;
}
@@ -2021,6 +2041,11 @@ qemuParseCommandLine(virCapsPtr caps,
goto error;
break;
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
+ if (qemuParseVxHSString(disk) < 0)
+ goto error;
+
+ break;
case VIR_STORAGE_NET_PROTOCOL_HTTP:
case VIR_STORAGE_NET_PROTOCOL_HTTPS:
case VIR_STORAGE_NET_PROTOCOL_FTP:
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index f0ed5c6..eb36694 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST,
"ftp",
"ftps",
"tftp",
- "ssh")
+ "ssh",
+ "vxhs")
VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST,
"tcp",
@@ -2719,6 +2720,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src,
case VIR_STORAGE_NET_PROTOCOL_ISCSI:
case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("malformed backing store path for protocol %s"),
protocol);
@@ -3219,6 +3221,65 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
return virStorageSourceParseBackingJSONInternal(src, json);
}
+#define QEMU_DEFAULT_VXHS_PORT "9999"
+
+static int
+virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
+ virJSONValuePtr json,
+ int opaque ATTRIBUTE_UNUSED)
+{
+ const char *uri = virJSONValueObjectGetString(json, "filename");
+ const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
+ virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
+ const char *hostname;
+ const char *port;
+
+ /* Check for legacy URI based syntax passed via 'filename' option */
+ if (uri) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("'VxHS' protocol does not support URI syntax"));
+ return -1;
+ }
+
+ if (!vdisk_id || !server) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("missing 'vdisk-id' or 'server' attribute in "
+ "JSON backing definition for VxHS volume"));
+ return -1;
+ }
+
+ hostname = virJSONValueObjectGetString(server, "host");
+ port = virJSONValueObjectGetString(server, "port");
+
+ if (!hostname) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("missing hostname for tcp backing server in "
+ "JSON backing definition for VxHS volume"));
+ return -1;
+ }
+
+ if (!port)
+ port = QEMU_DEFAULT_VXHS_PORT;
+
+ src->type = VIR_STORAGE_TYPE_NETWORK;
+ src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
+
+ if (VIR_STRDUP(src->path, vdisk_id) < 0)
+ return -1;
+
+ if (VIR_ALLOC_N(src->hosts, 1) < 0)
+ return -1;
+ src->nhosts = 1;
+
+ src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
+
+ if (VIR_STRDUP(src->hosts[0].name, hostname) < 0 ||
+ VIR_STRDUP(src->hosts[0].port, port) < 0)
+ return -1;
+
+ return 0;
+}
+
struct virStorageSourceJSONDriverParser {
const char *drvname;
int (*func)(virStorageSourcePtr src, virJSONValuePtr json, int opaque);
@@ -3241,6 +3302,7 @@ static const struct virStorageSourceJSONDriverParser jsonParsers[] = {
{"ssh", virStorageSourceParseBackingJSONSSH, 0},
{"rbd", virStorageSourceParseBackingJSONRBD, 0},
{"raw", virStorageSourceParseBackingJSONRaw, 0},
+ {"vxhs", virStorageSourceParseBackingJSONVxHS, 0},
};
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 0bff867..0b6e409 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -134,6 +134,7 @@ typedef enum {
VIR_STORAGE_NET_PROTOCOL_FTPS,
VIR_STORAGE_NET_PROTOCOL_TFTP,
VIR_STORAGE_NET_PROTOCOL_SSH,
+ VIR_STORAGE_NET_PROTOCOL_VXHS,
VIR_STORAGE_NET_PROTOCOL_LAST
} virStorageNetProtocol;
diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c
index cac440c..8bd6f3e 100644
--- a/src/xenconfig/xen_xl.c
+++ b/src/xenconfig/xen_xl.c
@@ -1024,6 +1024,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src)
case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
case VIR_STORAGE_NET_PROTOCOL_SSH:
+ case VIR_STORAGE_NET_PROTOCOL_VXHS:
case VIR_STORAGE_NET_PROTOCOL_LAST:
case VIR_STORAGE_NET_PROTOCOL_NONE:
virReportError(VIR_ERR_NO_SUPPORT,
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
new file mode 100644
index 0000000..f6e3e37
--- /dev/null
+++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
@@ -0,0 +1,24 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/libexec/qemu-kvm \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-cpu qemu32 \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\
+format=raw,if=none,id=drive-virtio-disk0,cache=none \
+-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
+id=virtio-disk0
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 1adbcfe..fc15714 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -50,6 +50,7 @@ static int testSanitizeDef(virDomainDefPtr vmdef)
typedef enum {
FLAG_EXPECT_WARNING = 1 << 0,
+ FLAG_EXPECT_FAIL = 1 << 1,
} virQemuXML2ArgvTestFlags;
static int testCompareXMLToArgvFiles(const char *xmlfile,
@@ -67,7 +68,16 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
if (!(vmdef = qemuParseCommandLineString(driver.caps, driver.xmlopt,
cmd, NULL, NULL, NULL)))
- goto fail;
+ {
+ if (flags & FLAG_EXPECT_FAIL) {
+ if (virTestLogContentAndReset() == NULL)
+ goto fail;
+
+ VIR_TEST_DEBUG("Got expected error from "
+ "qemuParseCommandLineString:\n");
+ goto out;
+ }
+ }
if (!virTestOOMActive()) {
if ((log = virTestLogContentAndReset()) == NULL)
@@ -106,6 +116,7 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
if (virTestCompareToFile(actualxml, xmlfile) < 0)
goto fail;
+ out:
ret = 0;
fail:
@@ -166,6 +177,9 @@ mymain(void)
# define DO_TEST(name) \
DO_TEST_FULL(name, 0)
+# define DO_TEST_FAIL(name) \
+ DO_TEST_FULL(name, FLAG_EXPECT_FAIL)
+
setenv("PATH", "/bin", 1);
setenv("USER", "test", 1);
setenv("LOGNAME", "test", 1);
@@ -220,6 +234,7 @@ mymain(void)
/* older format using CEPH_ARGS env var */
DO_TEST("disk-drive-network-rbd-ceph-env");
DO_TEST("disk-drive-network-sheepdog");
+ DO_TEST_FAIL("disk-drive-network-vxhs-fail");
DO_TEST("disk-usb");
DO_TEST("graphics-vnc");
DO_TEST("graphics-vnc-socket");
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
new file mode 100644
index 0000000..41dffff
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
@@ -0,0 +1,25 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/home/test \
+USER=test \
+LOGNAME=test \
+QEMU_AUDIO_DRV=none \
+/usr/bin/qemu-system-x86_64 \
+-name QEMUGuest1 \
+-S \
+-M pc \
+-cpu qemu32 \
+-m 214 \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-nographic \
+-nodefaults \
+-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
+-no-acpi \
+-boot c \
+-usb \
+-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
+file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\
+id=drive-virtio-disk0,cache=none \
+-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
+id=virtio-disk0
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
new file mode 100644
index 0000000..a488770
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
@@ -0,0 +1,34 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-x86_64</emulator>
+ <disk type='network' device='disk'>
+ <driver name='qemu' type='raw' cache='none'/>
+ <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
+ <host name='192.168.0.1' port='9999'/>
+ </source>
+ <backingStore/>
+ <target dev='vda' bus='virtio'/>
+ <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
+ <alias name='virtio-disk0'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 27eea70..0a1ef01 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -903,6 +903,7 @@ mymain(void)
# endif
DO_TEST("disk-drive-network-rbd-ipv6", NONE);
DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE);
+ DO_TEST("disk-drive-network-vxhs", NONE);
DO_TEST("disk-drive-no-boot",
QEMU_CAPS_BOOTINDEX);
DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid",
diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
index f344083..3a4e03b 100644
--- a/tests/virstoragetest.c
+++ b/tests/virstoragetest.c
@@ -1575,6 +1575,25 @@ mymain(void)
"<source protocol='sheepdog' name='test'>\n"
" <host name='example.com' port='321'/>\n"
"</source>\n");
+ TEST_BACKING_PARSE("json:{ \"file\": { "
+ "\"driver\": \"raw\","
+ "\"file\": {"
+ "\"driver\": \"file\","
+ "\"filename\": \"/path/to/file\" } } }",
+ "<source file='/path/to/file'/>\n");
+ TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\","
+ "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\","
+ "\"server\": { \"host\":\"example.com\","
+ "\"port\":\"1234\""
+ "}"
+ "}"
+ "}",
+ "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
+ " <host name='example.com' port='1234'/>\n"
+ "</source>\n");
+ TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\","
+ "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\""
+ "}", NULL);
#endif /* WITH_YAJL */
cleanup:
--
2.5.5
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote: > From: Ashish Mittal <ashish.mittal@veritas.com> > > Sample XML for a VxHS disk: > > <disk type='network' device='disk'> > <driver name='qemu' type='raw' cache='none'/> > <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'> > <host name='192.168.0.1' port='9999'/> > </source> > <backingStore/> > <target dev='vda' bus='virtio'/> > <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> > <alias name='virtio-disk0'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> > </disk> > > Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com> > --- [...] Additionally since libvirt supports QAPI introspection, this means we are now able to detect whether qemu supports vxhs and should report an error if it doesn't. You'll need to add a capability bit in qemu_capabilities.h and the detection string in qemu_capabilities.c to virQEMUCapsQMPSchemaQueries[]. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote:
> From: Ashish Mittal <ashish.mittal@veritas.com>
>
> Sample XML for a VxHS disk:
>
> <disk type='network' device='disk'>
> <driver name='qemu' type='raw' cache='none'/>
> <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
> <host name='192.168.0.1' port='9999'/>
> </source>
> <backingStore/>
> <target dev='vda' bus='virtio'/>
> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
> <alias name='virtio-disk0'/>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
> </disk>
>
> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> ---
> v2 changelog:
> (1) Added code for JSON parsing of a VxHS vdisk.
> (2) Added test case to verify JSON parsing.
> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>
> v3 changelog:
> (1) Implemented the modern syntax for VxHS disk specification.
> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
> (3) Added a negative test case to check failure when multiple hosts
> are specified for a VxHS disk.
>
> v4 changelog:
> (1) Fixes per review comments from v3.
> (2) Had to remove a test from the previous version that checked for
> error when multiple hosts are specified for VxHS device.
> This started failing virschematest with the error
> "XML document failed to validate against schema" as the
> docs/schemas/domain.rng specifies only a single host.
>
> docs/formatdomain.html.in | 15 ++++-
> docs/schemas/domaincommon.rng | 13 ++++
> src/libxl/libxl_conf.c | 1 +
> src/qemu/qemu_command.c | 70 ++++++++++++++++++++++
> src/qemu/qemu_driver.c | 3 +
> src/qemu/qemu_parse_command.c | 25 ++++++++
> src/util/virstoragefile.c | 64 +++++++++++++++++++-
> src/util/virstoragefile.h | 1 +
> src/xenconfig/xen_xl.c | 1 +
> .../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24 ++++++++
> tests/qemuargv2xmltest.c | 17 +++++-
> .../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++
> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++
> tests/qemuxml2argvtest.c | 1 +
> tests/virstoragetest.c | 19 ++++++
> 15 files changed, 308 insertions(+), 5 deletions(-)
> create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 36bea67..62d67f4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
[...]
> @@ -2511,6 +2511,9 @@
> target's name by a slash (e.g.,
> <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not
> specified, the default LUN is zero.
> + For "vxhs" (<span class="since">since 3.3.0</span>), thea
3.6.0 at best.
> + <code>name</code> is the UUID of the volume, assigned by the
> + HyperScale sever.
> <span class="since">Since 0.8.7</span>
> </dd>
> <dt><code>volume</code></dt>
[...]
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index bdf7103..7525a2a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1613,6 +1613,18 @@
> </element>
> </define>
>
> + <define name="diskSourceNetworkProtocolVxHS">
> + <element name="source">
> + <attribute name="protocol">
> + <choice>
> + <value>vxhs</value>
> + </choice>
> + </attribute>
> + <attribute name="name"/>
> + <ref name="diskSourceNetworkHost"/>
This is misaligned.
> + </element>
> + </define>
> +
> <define name="diskSourceNetwork">
> <attribute name="type">
> <value>network</value>
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c53ab97..8e00782 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol,
> return 0;
>
> case VIR_STORAGE_NET_PROTOCOL_RBD:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> case VIR_STORAGE_NET_PROTOCOL_NONE:
> /* not applicable */
Now we are in the section of stuff which should be split into a separate
patch.
Also here you declare that there,s no default port ... (and you said)
> @@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
> }
>
>
> +#define QEMU_DEFAULT_VXHS_PORT "9999"
And here you declare the default port via a ... define. I'd suggest you
stick with the common code paths.
> +
> +/* Build the VxHS host object */
If you want to add comments, please make them useful. Document
arguments, and return values.
> +static virJSONValuePtr
> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
> +{
> + virJSONValuePtr server = NULL;
> + virStorageNetHostDefPtr host;
> + const char *portstr;
> +
> + if (src->nhosts != 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("protocol VxHS accepts only one host"));
> + goto cleanup;
> + }
> +
> + host = src->hosts;
> + portstr = host->port;
> +
> + if (!portstr)
> + portstr = QEMU_DEFAULT_VXHS_PORT;
> +
> + if (virJSONValueObjectCreate(&server,
> + "s:host", host->name,
> + "s:port", portstr,
> + NULL) < 0)
This looks like it's generating a server structure filled with
'InetSocketAddressBase' qemu types. Since vxhs is not the only one using
those, you can make this a generic function.
Additionally it should return an array of those object in case when
there's more than one host. This function should not fill any default
ports, that's what the caller should do.
> + server = NULL;
> +
> + cleanup:
Why have a cleanup label if there's no cleanup code?
> + return server;
> +}
> +
> +
[...]
> @@ -1136,6 +1196,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
> ret = virBufferContentAndReset(&buf);
> break;
>
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("'VxHS' protocol does not support URI syntax"));
> + goto cleanup;
Note that this will cause that external snapshots will not work with
VXHS. I'm currently dealing with the same problem with multi-host
gluster disks.
> +
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("'ssh' protocol is not yet supported"));
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cdb727b..d43de69 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
[...]
> @@ -13746,6 +13747,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
> case VIR_STORAGE_NET_PROTOCOL_FTPS:
> case VIR_STORAGE_NET_PROTOCOL_TFTP:
> case VIR_STORAGE_NET_PROTOCOL_SSH:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> case VIR_STORAGE_NET_PROTOCOL_LAST:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("external active snapshots are not supported on "
Okay, fair enough, you expressly declare that you don't need them.
> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
> index af9063c..aa15225 100644
> --- a/src/qemu/qemu_parse_command.c
> +++ b/src/qemu/qemu_parse_command.c
> @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk)
> return -1;
> }
>
> +static int
> +qemuParseVxHSString(virDomainDiskDefPtr def)
> +{
> + virURIPtr uri = NULL;
> +
> + if (!(uri = virURIParse(def->src->path)))
> + return -1;
> +
> + return qemuParseDriveURIString(def, uri, "vxhs");
Above, you've declared that URI strings are not supported by libvirt, I
don't feel we need to parse them then.
> +}
> +
>
> /*
> * This method takes a string representing a QEMU command line ARGV set
> @@ -737,6 +748,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
> if (VIR_STRDUP(def->src->path, vdi) < 0)
> goto error;
> }
> + } else if (STRPREFIX(def->src->path, "vxhs:")) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("VxHS protocol does not support URI syntax '%s'"),
> + def->src->path);
Umm, how is this even supposed to work then? Above you explicitly parse
it as an URI
> + goto error;
> } else {
> def->src->type = VIR_STORAGE_TYPE_FILE;
> }
[...]
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index f0ed5c6..eb36694 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST,
> "ftp",
> "ftps",
> "tftp",
> - "ssh")
> + "ssh",
> + "vxhs")
>
> VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST,
> "tcp",
This belongs to the patch adding vxhs globally.
[...]
> @@ -3219,6 +3221,65 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
> return virStorageSourceParseBackingJSONInternal(src, json);
> }
>
> +#define QEMU_DEFAULT_VXHS_PORT "9999"
Again?!?. No. Define this globally.
> +
> +static int
> +virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
> + virJSONValuePtr json,
> + int opaque ATTRIBUTE_UNUSED)
> +{
> + const char *uri = virJSONValueObjectGetString(json, "filename");
> + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
> + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
> + const char *hostname;
> + const char *port;
> +
> + /* Check for legacy URI based syntax passed via 'filename' option */
> + if (uri) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("'VxHS' protocol does not support URI syntax"));
> + return -1;
> + }
This never was supported in libvirt, so it's not necessary. Also in
qemu this was added post 2.9.0 so I doubt it ever supported the
'filename' syntax.
> +
> + if (!vdisk_id || !server) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("missing 'vdisk-id' or 'server' attribute in "
> + "JSON backing definition for VxHS volume"));
> + return -1;
> + }
> +
> + hostname = virJSONValueObjectGetString(server, "host");
> + port = virJSONValueObjectGetString(server, "port");
Use virStorageSourceParseBackingJSONInetSocketAddress instead of
hand-rolling the code.
> +
> + if (!hostname) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("missing hostname for tcp backing server in "
> + "JSON backing definition for VxHS volume"));
> + return -1;
> + }
> +
> + if (!port)
> + port = QEMU_DEFAULT_VXHS_PORT;
This should not fill the defaults. This code should parse what's in the
backing store.
[...]
> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
> new file mode 100644
> index 0000000..f6e3e37
> --- /dev/null
> +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
> @@ -0,0 +1,24 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/libexec/qemu-kvm \
> +-name QEMUGuest1 \
> +-S \
> +-M pc \
> +-cpu qemu32 \
> +-m 214 \
> +-smp 1,sockets=1,cores=1,threads=1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\
There are at least 4 places where you say that URI syntax is not working
....
> +format=raw,if=none,id=drive-virtio-disk0,cache=none \
> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
> +id=virtio-disk0
> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
> index 1adbcfe..fc15714 100644
> --- a/tests/qemuargv2xmltest.c
> +++ b/tests/qemuargv2xmltest.c
> @@ -50,6 +50,7 @@ static int testSanitizeDef(virDomainDefPtr vmdef)
[...]
If you need to make functional changes to the test suite, please put
them into a separate patch.
>
> typedef enum {
> FLAG_EXPECT_WARNING = 1 << 0,
> + FLAG_EXPECT_FAIL = 1 << 1,
> } virQemuXML2ArgvTestFlags;
>
> static int testCompareXMLToArgvFiles(const char *xmlfile,
> @@ -67,7 +68,16 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
>
> if (!(vmdef = qemuParseCommandLineString(driver.caps, driver.xmlopt,
> cmd, NULL, NULL, NULL)))
> - goto fail;
> + {
> + if (flags & FLAG_EXPECT_FAIL) {
> + if (virTestLogContentAndReset() == NULL)
This leaks the log buffer if it returns non-null.
> + goto fail;
> +
> + VIR_TEST_DEBUG("Got expected error from "
> + "qemuParseCommandLineString:\n");
> + goto out;
> + }
> + }
>
> if (!virTestOOMActive()) {
> if ((log = virTestLogContentAndReset()) == NULL)
> @@ -106,6 +116,7 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
> if (virTestCompareToFile(actualxml, xmlfile) < 0)
> goto fail;
>
> + out:
> ret = 0;
>
> fail:
The fail label here should be called 'cleanup' per our standard, thus
you don't need to add any new label. Just set ret to 0 and jump here.
> @@ -166,6 +177,9 @@ mymain(void)
> # define DO_TEST(name) \
> DO_TEST_FULL(name, 0)
>
> +# define DO_TEST_FAIL(name) \
> + DO_TEST_FULL(name, FLAG_EXPECT_FAIL)
> +
> setenv("PATH", "/bin", 1);
> setenv("USER", "test", 1);
> setenv("LOGNAME", "test", 1);
> @@ -220,6 +234,7 @@ mymain(void)
> /* older format using CEPH_ARGS env var */
> DO_TEST("disk-drive-network-rbd-ceph-env");
> DO_TEST("disk-drive-network-sheepdog");
> + DO_TEST_FAIL("disk-drive-network-vxhs-fail");
> DO_TEST("disk-usb");
> DO_TEST("graphics-vnc");
> DO_TEST("graphics-vnc-socket");
[...]
> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
> index f344083..3a4e03b 100644
> --- a/tests/virstoragetest.c
> +++ b/tests/virstoragetest.c
> @@ -1575,6 +1575,25 @@ mymain(void)
> "<source protocol='sheepdog' name='test'>\n"
> " <host name='example.com' port='321'/>\n"
> "</source>\n");
> + TEST_BACKING_PARSE("json:{ \"file\": { "
> + "\"driver\": \"raw\","
> + "\"file\": {"
> + "\"driver\": \"file\","
> + "\"filename\": \"/path/to/file\" } } }",
> + "<source file='/path/to/file'/>\n");
This does not seem relevant. Wrong resolution of a merge conflict
perhaps?
> + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\","
> + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\","
> + "\"server\": { \"host\":\"example.com\","
> + "\"port\":\"1234\""
> + "}"
> + "}"
> + "}",
> + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
> + " <host name='example.com' port='1234'/>\n"
> + "</source>\n");
> + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\","
> + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\""
> + "}", NULL);
So if this is not supposed to work, why bother adding it at all?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi Peter,
Patch series V5 addresses most of the review comments from this email.
I'm working on the other set of review comments from you and John, and
will send updates soon.
On Fri, Jun 30, 2017 at 1:22 AM, Peter Krempa <pkrempa@redhat.com> wrote:
> On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote:
>> From: Ashish Mittal <ashish.mittal@veritas.com>
>>
>> Sample XML for a VxHS disk:
>>
>> <disk type='network' device='disk'>
>> <driver name='qemu' type='raw' cache='none'/>
>> <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'>
>> <host name='192.168.0.1' port='9999'/>
>> </source>
>> <backingStore/>
>> <target dev='vda' bus='virtio'/>
>> <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial>
>> <alias name='virtio-disk0'/>
>> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
>> </disk>
>>
>> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>> ---
>> v2 changelog:
>> (1) Added code for JSON parsing of a VxHS vdisk.
>> (2) Added test case to verify JSON parsing.
>> (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS.
>> (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.
>>
>> v3 changelog:
>> (1) Implemented the modern syntax for VxHS disk specification.
>> (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax.
>> (3) Added a negative test case to check failure when multiple hosts
>> are specified for a VxHS disk.
>>
>> v4 changelog:
>> (1) Fixes per review comments from v3.
>> (2) Had to remove a test from the previous version that checked for
>> error when multiple hosts are specified for VxHS device.
>> This started failing virschematest with the error
>> "XML document failed to validate against schema" as the
>> docs/schemas/domain.rng specifies only a single host.
>>
>> docs/formatdomain.html.in | 15 ++++-
>> docs/schemas/domaincommon.rng | 13 ++++
>> src/libxl/libxl_conf.c | 1 +
>> src/qemu/qemu_command.c | 70 ++++++++++++++++++++++
>> src/qemu/qemu_driver.c | 3 +
>> src/qemu/qemu_parse_command.c | 25 ++++++++
>> src/util/virstoragefile.c | 64 +++++++++++++++++++-
>> src/util/virstoragefile.h | 1 +
>> src/xenconfig/xen_xl.c | 1 +
>> .../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24 ++++++++
>> tests/qemuargv2xmltest.c | 17 +++++-
>> .../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++
>> .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++
>> tests/qemuxml2argvtest.c | 1 +
>> tests/virstoragetest.c | 19 ++++++
>> 15 files changed, 308 insertions(+), 5 deletions(-)
>> create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args
>> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 36bea67..62d67f4 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>
> [...]
>
>> @@ -2511,6 +2511,9 @@
>> target's name by a slash (e.g.,
>> <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not
>> specified, the default LUN is zero.
>> + For "vxhs" (<span class="since">since 3.3.0</span>), thea
>
> 3.6.0 at best.
Changed to 3.8.0 in all places.
>
>> + <code>name</code> is the UUID of the volume, assigned by the
>> + HyperScale sever.
>> <span class="since">Since 0.8.7</span>
>> </dd>
>> <dt><code>volume</code></dt>
>
> [...]
>
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index bdf7103..7525a2a 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -1613,6 +1613,18 @@
>> </element>
>> </define>
>>
>> + <define name="diskSourceNetworkProtocolVxHS">
>> + <element name="source">
>> + <attribute name="protocol">
>> + <choice>
>> + <value>vxhs</value>
>> + </choice>
>> + </attribute>
>> + <attribute name="name"/>
>> + <ref name="diskSourceNetworkHost"/>
>
> This is misaligned.
>
Fixed.
>> + </element>
>> + </define>
>> +
>> <define name="diskSourceNetwork">
>> <attribute name="type">
>> <value>network</value>
>
> [...]
>
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index c53ab97..8e00782 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol,
>> return 0;
>>
>> case VIR_STORAGE_NET_PROTOCOL_RBD:
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> case VIR_STORAGE_NET_PROTOCOL_LAST:
>> case VIR_STORAGE_NET_PROTOCOL_NONE:
>> /* not applicable */
>
> Now we are in the section of stuff which should be split into a separate
> patch.
>
> Also here you declare that there,s no default port ... (and you said)
>
qemuNetworkDriveGetPort() NA in the latest code. Added to
virStorageSourceNetworkDefaultPort()
>> @@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src)
>> }
>>
>>
>> +#define QEMU_DEFAULT_VXHS_PORT "9999"
>
> And here you declare the default port via a ... define. I'd suggest you
> stick with the common code paths.
>
The now-defunct qemuNetworkDriveGetPort() was not used in the JSON
code path, hence I had omitted it. Added to
virStorageSourceNetworkDefaultPort() in the latest code.
>> +
>> +/* Build the VxHS host object */
>
> If you want to add comments, please make them useful. Document
> arguments, and return values.
>
>> +static virJSONValuePtr
>> +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src)
>> +{
>> + virJSONValuePtr server = NULL;
>> + virStorageNetHostDefPtr host;
>> + const char *portstr;
>> +
>> + if (src->nhosts != 1) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("protocol VxHS accepts only one host"));
>> + goto cleanup;
>> + }
>> +
>> + host = src->hosts;
>> + portstr = host->port;
>> +
>> + if (!portstr)
>> + portstr = QEMU_DEFAULT_VXHS_PORT;
>> +
>> + if (virJSONValueObjectCreate(&server,
>> + "s:host", host->name,
>> + "s:port", portstr,
>> + NULL) < 0)
>
>
> This looks like it's generating a server structure filled with
> 'InetSocketAddressBase' qemu types. Since vxhs is not the only one using
> those, you can make this a generic function.
>
I hope this can be deferred for later. I guess this will affect
non-vxhs protocols as well.
> Additionally it should return an array of those object in case when
> there's more than one host. This function should not fill any default
> ports, that's what the caller should do.
>
>> + server = NULL;
>> +
>> + cleanup:
>
> Why have a cleanup label if there's no cleanup code?
>
Changed.
>> + return server;
>> +}
>> +
>> +
>
> [...]
>
>> @@ -1136,6 +1196,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>> ret = virBufferContentAndReset(&buf);
>> break;
>>
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("'VxHS' protocol does not support URI syntax"));
>> + goto cleanup;
>
> Note that this will cause that external snapshots will not work with
> VXHS. I'm currently dealing with the same problem with multi-host
> gluster disks.
>
>> +
>> case VIR_STORAGE_NET_PROTOCOL_SSH:
>> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> _("'ssh' protocol is not yet supported"));
>
> [...]
>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index cdb727b..d43de69 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>
> [...]
>
>> @@ -13746,6 +13747,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d
>> case VIR_STORAGE_NET_PROTOCOL_FTPS:
>> case VIR_STORAGE_NET_PROTOCOL_TFTP:
>> case VIR_STORAGE_NET_PROTOCOL_SSH:
>> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
>> case VIR_STORAGE_NET_PROTOCOL_LAST:
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("external active snapshots are not supported on "
>
> Okay, fair enough, you expressly declare that you don't need them.
>
>> diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c
>> index af9063c..aa15225 100644
>> --- a/src/qemu/qemu_parse_command.c
>> +++ b/src/qemu/qemu_parse_command.c
>> @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk)
>> return -1;
>> }
>>
>> +static int
>> +qemuParseVxHSString(virDomainDiskDefPtr def)
>> +{
>> + virURIPtr uri = NULL;
>> +
>> + if (!(uri = virURIParse(def->src->path)))
>> + return -1;
>> +
>> + return qemuParseDriveURIString(def, uri, "vxhs");
>
> Above, you've declared that URI strings are not supported by libvirt, I
> don't feel we need to parse them then.
>
Removed.
>> +}
>> +
>>
>> /*
>> * This method takes a string representing a QEMU command line ARGV set
>> @@ -737,6 +748,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt,
>> if (VIR_STRDUP(def->src->path, vdi) < 0)
>> goto error;
>> }
>> + } else if (STRPREFIX(def->src->path, "vxhs:")) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("VxHS protocol does not support URI syntax '%s'"),
>> + def->src->path);
>
> Umm, how is this even supposed to work then? Above you explicitly parse
> it as an URI
>
Changed accordingly.
>> + goto error;
>> } else {
>> def->src->type = VIR_STORAGE_TYPE_FILE;
>> }
>
> [...]
>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index f0ed5c6..eb36694 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST,
>> "ftp",
>> "ftps",
>> "tftp",
>> - "ssh")
>> + "ssh",
>> + "vxhs")
>>
>> VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST,
>> "tcp",
>
> This belongs to the patch adding vxhs globally.
>
This is the patch adding vxhs functionality. Hopefully this is the
correct place for this!
>
> [...]
>
>> @@ -3219,6 +3221,65 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src,
>> return virStorageSourceParseBackingJSONInternal(src, json);
>> }
>>
>> +#define QEMU_DEFAULT_VXHS_PORT "9999"
>
> Again?!?. No. Define this globally.
>
Removed.
>> +
>> +static int
>> +virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
>> + virJSONValuePtr json,
>> + int opaque ATTRIBUTE_UNUSED)
>> +{
>> + const char *uri = virJSONValueObjectGetString(json, "filename");
>> + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
>> + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
>> + const char *hostname;
>> + const char *port;
>> +
>> + /* Check for legacy URI based syntax passed via 'filename' option */
>> + if (uri) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("'VxHS' protocol does not support URI syntax"));
>> + return -1;
>> + }
>
> This never was supported in libvirt, so it's not necessary. Also in
> qemu this was added post 2.9.0 so I doubt it ever supported the
> 'filename' syntax.
>
Removed.
>> +
>> + if (!vdisk_id || !server) {
>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("missing 'vdisk-id' or 'server' attribute in "
>> + "JSON backing definition for VxHS volume"));
>> + return -1;
>> + }
>> +
>> + hostname = virJSONValueObjectGetString(server, "host");
>> + port = virJSONValueObjectGetString(server, "port");
>
> Use virStorageSourceParseBackingJSONInetSocketAddress instead of
> hand-rolling the code.
>
Changed accordingly.
>> +
>> + if (!hostname) {
>> + virReportError(VIR_ERR_INVALID_ARG, "%s",
>> + _("missing hostname for tcp backing server in "
>> + "JSON backing definition for VxHS volume"));
>> + return -1;
>> + }
>> +
>> + if (!port)
>> + port = QEMU_DEFAULT_VXHS_PORT;
>
> This should not fill the defaults. This code should parse what's in the
> backing store.
>
Changed.
> [...]
>
>> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
>> new file mode 100644
>> index 0000000..f6e3e37
>> --- /dev/null
>> +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args
>> @@ -0,0 +1,24 @@
>> +LC_ALL=C \
>> +PATH=/bin \
>> +HOME=/home/test \
>> +USER=test \
>> +LOGNAME=test \
>> +QEMU_AUDIO_DRV=none \
>> +/usr/libexec/qemu-kvm \
>> +-name QEMUGuest1 \
>> +-S \
>> +-M pc \
>> +-cpu qemu32 \
>> +-m 214 \
>> +-smp 1,sockets=1,cores=1,threads=1 \
>> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
>> +-nographic \
>> +-nodefaults \
>> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
>> +-no-acpi \
>> +-boot c \
>> +-usb \
>> +-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\
>
> There are at least 4 places where you say that URI syntax is not working
> ....
This test case was to test failure when URI is encountered, but I see
your point. Since the URI syntax will never exist, we don't need to
test it.
>
>> +format=raw,if=none,id=drive-virtio-disk0,cache=none \
>> +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\
>> +id=virtio-disk0
>> diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
>> index 1adbcfe..fc15714 100644
>> --- a/tests/qemuargv2xmltest.c
>> +++ b/tests/qemuargv2xmltest.c
>> @@ -50,6 +50,7 @@ static int testSanitizeDef(virDomainDefPtr vmdef)
>
> [...]
>
> If you need to make functional changes to the test suite, please put
> them into a separate patch.
>
The above failure test has been removed from qemuargv2xmltest.
Therefore the test fail changes are no longer needed. Removed.
>>
>> typedef enum {
>> FLAG_EXPECT_WARNING = 1 << 0,
>> + FLAG_EXPECT_FAIL = 1 << 1,
>> } virQemuXML2ArgvTestFlags;
>>
>> static int testCompareXMLToArgvFiles(const char *xmlfile,
>> @@ -67,7 +68,16 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
>>
>> if (!(vmdef = qemuParseCommandLineString(driver.caps, driver.xmlopt,
>> cmd, NULL, NULL, NULL)))
>> - goto fail;
>> + {
>> + if (flags & FLAG_EXPECT_FAIL) {
>> + if (virTestLogContentAndReset() == NULL)
>
> This leaks the log buffer if it returns non-null.
NA now.
>
>> + goto fail;
>> +
>> + VIR_TEST_DEBUG("Got expected error from "
>> + "qemuParseCommandLineString:\n");
>> + goto out;
>> + }
>> + }
>>
>> if (!virTestOOMActive()) {
>> if ((log = virTestLogContentAndReset()) == NULL)
>> @@ -106,6 +116,7 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,
>> if (virTestCompareToFile(actualxml, xmlfile) < 0)
>> goto fail;
>>
>> + out:
>> ret = 0;
>>
>> fail:
>
> The fail label here should be called 'cleanup' per our standard, thus
> you don't need to add any new label. Just set ret to 0 and jump here.
>
NA anymore.
>> @@ -166,6 +177,9 @@ mymain(void)
>> # define DO_TEST(name) \
>> DO_TEST_FULL(name, 0)
>>
>> +# define DO_TEST_FAIL(name) \
>> + DO_TEST_FULL(name, FLAG_EXPECT_FAIL)
>> +
>> setenv("PATH", "/bin", 1);
>> setenv("USER", "test", 1);
>> setenv("LOGNAME", "test", 1);
>> @@ -220,6 +234,7 @@ mymain(void)
>> /* older format using CEPH_ARGS env var */
>> DO_TEST("disk-drive-network-rbd-ceph-env");
>> DO_TEST("disk-drive-network-sheepdog");
>> + DO_TEST_FAIL("disk-drive-network-vxhs-fail");
>> DO_TEST("disk-usb");
>> DO_TEST("graphics-vnc");
>> DO_TEST("graphics-vnc-socket");
>
> [...]
>
>> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c
>> index f344083..3a4e03b 100644
>> --- a/tests/virstoragetest.c
>> +++ b/tests/virstoragetest.c
>> @@ -1575,6 +1575,25 @@ mymain(void)
>> "<source protocol='sheepdog' name='test'>\n"
>> " <host name='example.com' port='321'/>\n"
>> "</source>\n");
>> + TEST_BACKING_PARSE("json:{ \"file\": { "
>> + "\"driver\": \"raw\","
>> + "\"file\": {"
>> + "\"driver\": \"file\","
>> + "\"filename\": \"/path/to/file\" } } }",
>> + "<source file='/path/to/file'/>\n");
>
> This does not seem relevant. Wrong resolution of a merge conflict
> perhaps?
>
Yes. I have removed this now.
>> + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\","
>> + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\","
>> + "\"server\": { \"host\":\"example.com\","
>> + "\"port\":\"1234\""
>> + "}"
>> + "}"
>> + "}",
>> + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n"
>> + " <host name='example.com' port='1234'/>\n"
>> + "</source>\n");
>> + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\","
>> + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\""
>> + "}", NULL);
>
> So if this is not supposed to work, why bother adding it at all?
Removed.
Thanks,
Ashish
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.