On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote:
> From: Ashish Mittal <ashish.mittal@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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?
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
