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@xxxxxxxxxx> wrote:
> 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.
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
