Original implementation used 'SocketAddress' equivalent from qemu for the disk server field, while qemu documentation specifies 'InetSocketAddress'. The backing store parser uses the correct parsing function but the formatter used the incorrect one (and also with the legacy mode enabled which was wrong). --- src/qemu/qemu_block.c | 36 +++++++++++++++++++++- ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 8 ++--- .../qemuxml2argv-disk-drive-network-vxhs.args | 4 +-- tests/virstoragetest.c | 3 +- 4 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 0761f8991..96db19226 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -501,6 +501,40 @@ qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src, } +/** + * qemuBlockStorageSourceBuildJSONInetSocketAddress + * @host: the virStorageNetHostDefPtr definition to build + * + * Formats @hosts into a json object conforming to the 'InetSocketAddress' type + * in qemu. + * + * Returns a virJSONValuePtr for a single server. + */ +static virJSONValuePtr +qemuBlockStorageSourceBuildJSONInetSocketAddress(virStorageNetHostDefPtr host) +{ + virJSONValuePtr ret = NULL; + char *port = NULL; + + if (host->transport != VIR_STORAGE_NET_HOST_TRANS_TCP) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("only TCP protocol can be converted to InetSocketAddress")); + return NULL; + } + + if (virAsprintf(&port, "%u", host->port) < 0) + return NULL; + + ignore_value(virJSONValueObjectCreate(&ret, + "s:host", host->name, + "s:port", port, + NULL)); + + VIR_FREE(port); + return ret; +} + + static virJSONValuePtr qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) { @@ -540,7 +574,7 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src) return NULL; } - if (!(server = qemuBlockStorageSourceBuildJSONSocketAddress(src->hosts, true))) + if (!(server = qemuBlockStorageSourceBuildJSONInetSocketAddress(&src->hosts[0]))) return NULL; /* VxHS disk specification example: diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args index a75272454..eaa8699a9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -23,7 +23,7 @@ server,nowait \ -object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/libvirt-vxhs,\ endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\ -file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,file.server.type=tcp,\ +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,\ @@ -31,13 +31,13 @@ id=virtio-disk0 \ -object tls-creds-x509,id=objvirtio-disk1_tls0,dir=/etc/pki/libvirt-vxhs,\ endpoint=client,verify-peer=yes \ -drive file.driver=vxhs,file.tls-creds=objvirtio-disk1_tls0,\ -file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,file.server.type=tcp,\ +file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\ file.server.host=192.168.0.2,file.server.port=9999,format=raw,if=none,\ id=drive-virtio-disk1,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ id=virtio-disk1 \ -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\ -file.server.type=tcp,file.server.host=192.168.0.3,file.server.port=9999,\ -format=raw,if=none,id=drive-virtio-disk2,cache=none \ +file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk2,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ id=virtio-disk2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args index b62ace3de..1747dc80f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args @@ -21,7 +21,7 @@ server,nowait \ -boot c \ -usb \ -drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ -file.server.type=tcp,file.server.host=192.168.0.1,file.server.port=9999,\ -format=raw,if=none,id=drive-virtio-disk0,cache=none \ +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/virstoragetest.c b/tests/virstoragetest.c index e1d875172..78f24ad78 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1601,8 +1601,7 @@ mymain(void) "</source>\n"); TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," - "\"server\": { \"type\":\"tcp\"," - "\"host\":\"example.com\"," + "\"server\": { \"host\":\"example.com\"," "\"port\":\"9999\"" "}" "}" -- 2.14.3

On Fri, Nov 03, 2017 at 03:29:19PM +0100, Peter Krempa wrote: Please put the */ on a separate line. Jan

Similarly to how we store gluster names, split the name into a pool and image portions when paring the XML and store them separately. --- src/conf/domain_conf.c | 13 +++++++------ src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_command.c | 2 +- src/util/virstoragefile.c | 22 +++++++++++----------- src/xenconfig/xen_xl.c | 2 +- tests/domainsnapshotxml2xmlin/disk_snapshot.xml | 2 +- tests/domainsnapshotxml2xmlout/disk_snapshot.xml | 2 +- 7 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01cd3811c..a7fe1ac85 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8383,16 +8383,17 @@ virDomainDiskSourceNetworkParse(xmlNodePtr node, src->tlsFromConfig = !!tlsCfgVal; } - /* for historical reasons the volume name for gluster volume is stored - * as a part of the path. This is hard to work with when dealing with - * relative names. Split out the volume into a separate variable */ - if (src->path && src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER) { + /* for historical reasons we store the volume and image name in one XML + * element although it complicates thing when attempting to access them. */ + if (src->path && + (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER || + src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) { char *tmp; if (!(tmp = strchr(src->path, '/')) || tmp == src->path) { virReportError(VIR_ERR_XML_ERROR, - _("missing volume name or file name in " - "gluster source path '%s'"), src->path); + _("can't split path '%s' into pool name and image " + "name"), src->path); goto cleanup; } diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index ecbabfc79..63397e94c 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -657,7 +657,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src, goto cleanup; } - virBufferStrcat(&buf, "rbd:", src->path, NULL); + virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL); if (username) { virBufferEscape(&buf, '\\', ":", ":id=%s", username); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 25d5fdf18..0dcdc8976 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -951,7 +951,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, goto cleanup; } - virBufferStrcat(&buf, "rbd:", src->path, NULL); + virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL); if (src->snapshot) virBufferEscape(&buf, '\\', ":", "@%s", src->snapshot); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d48358abb..9cee64312 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2545,6 +2545,14 @@ virStorageSourceParseRBDColonString(const char *rbdstr, *p = '\0'; } + /* pool vs. image name */ + if ((p = strchr(src->path, '/'))) { + VIR_STEAL_PTR(src->volume, src->path); + if (VIR_STRDUP(src->path, p + 1) < 0) + goto error; + *p = '\0'; + } + /* options */ if (!options) return 0; /* all done */ @@ -3178,7 +3186,6 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, const char *conf = virJSONValueObjectGetString(json, "conf"); const char *snapshot = virJSONValueObjectGetString(json, "snapshot"); virJSONValuePtr servers = virJSONValueObjectGetArray(json, "server"); - char *fullname = NULL; size_t nservers; size_t i; int ret = -1; @@ -3197,17 +3204,12 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, return -1; } - /* currently we need to store the pool name and image name together, since - * the rest of the code is not prepared for it */ - if (virAsprintf(&fullname, "%s/%s", pool, image) < 0) - return -1; - - if (VIR_STRDUP(src->snapshot, snapshot) < 0 || + if (VIR_STRDUP(src->volume, pool) < 0 || + VIR_STRDUP(src->path, image) < 0 || + VIR_STRDUP(src->snapshot, snapshot) < 0 || VIR_STRDUP(src->configFile, conf) < 0) goto cleanup; - VIR_STEAL_PTR(src->path, fullname); - if (servers) { nservers = virJSONValueArraySize(servers); @@ -3225,8 +3227,6 @@ virStorageSourceParseBackingJSONRBD(virStorageSourcePtr src, ret = 0; cleanup: - VIR_FREE(fullname); - return ret; } diff --git a/src/xenconfig/xen_xl.c b/src/xenconfig/xen_xl.c index 8acbfe3f6..a61f1d7d5 100644 --- a/src/xenconfig/xen_xl.c +++ b/src/xenconfig/xen_xl.c @@ -1040,7 +1040,7 @@ xenFormatXLDiskSrcNet(virStorageSourcePtr src) goto cleanup; } - virBufferStrcat(&buf, "rbd:", src->path, NULL); + virBufferStrcat(&buf, "rbd:", src->volume, "/", src->path, NULL); virBufferAddLit(&buf, ":auth_supported=none"); diff --git a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml index aa1522a45..cf5ea0814 100644 --- a/tests/domainsnapshotxml2xmlin/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlin/disk_snapshot.xml @@ -24,7 +24,7 @@ </source> </disk> <disk name='hdh' snapshot='external' type='network'> - <source protocol='rbd' name='name'> + <source protocol='rbd' name='vol/name'> <host name='host' port='1234'/> <host name='host2' port='1234' transport='rdma'/> <host name='host3' port='1234'/> diff --git a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml index c2e77d7ac..0e81f35c6 100644 --- a/tests/domainsnapshotxml2xmlout/disk_snapshot.xml +++ b/tests/domainsnapshotxml2xmlout/disk_snapshot.xml @@ -23,7 +23,7 @@ </source> </disk> <disk name='hdh' snapshot='external' type='network'> - <source protocol='rbd' name='name'> + <source protocol='rbd' name='vol/name'> <host name='host' port='1234'/> <host name='host2' port='1234' transport='rdma'/> <host name='host3' port='1234'/> -- 2.14.3

On Fri, Nov 03, 2017 at 03:29:22PM +0100, Peter Krempa wrote: proxy-password-secret Jan

On Tue, Nov 07, 2017 at 04:04:02PM +0100, Peter Krempa wrote: ACK Thanks, patches are welcome. Jan

On 11/03/2017 10:29 AM, Peter Krempa wrote: Coverity notes that serverprops will be leaked on failure Actually it REALLY doesn't like the "a:server" consumption, but I already have quite a few of those in my false positive list. John

On Fri, Nov 03, 2017 at 03:29:25PM +0100, Peter Krempa wrote: Indentation is off here. Jan

On 11/03/2017 10:29 AM, Peter Krempa wrote: Again Coverity here w/ serverprops being leaked on failure. John

Format out the node-name if it was assigned for JSON-based storage specification. --- src/qemu/qemu_block.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 6f6d294bf..6df0dc0fb 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -1005,5 +1005,10 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src) break; } + if (virJSONValueObjectAdd(fileprops, "S:node-name", src->nodestorage, NULL) < 0) { + virJSONValueFree(fileprops); + return NULL; + } + return fileprops; } -- 2.14.3

Add a new test program called 'qemublocktest' to test the block layer related stuff and test storage source to JSON generator by comparing it to the JSON parser. --- tests/Makefile.am | 14 +++- tests/qemublocktest.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 tests/qemublocktest.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 0b2305d70..939acb0a1 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -282,7 +282,9 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \ qemumonitortest qemumonitorjsontest qemuhotplugtest \ qemuagenttest qemucapabilitiestest qemucaps2xmltest \ qemumemlocktest \ - qemucommandutiltest + qemucommandutiltest \ + qemublocktest \ + $(NULL) test_helpers += qemucapsprobe test_libraries += libqemumonitortestutils.la \ libqemutestdriver.la \ @@ -668,6 +670,15 @@ qemuhotplugtest_SOURCES = \ $(NULL) qemuhotplugtest_LDADD = libqemumonitortestutils.la $(qemu_LDADDS) $(LDADDS) +qemublocktest_SOURCES = \ + qemublocktest.c testutils.h testutils.c +qemublocktest_LDADD = $(LDADDS) \ + ../src/libvirt_conf.la \ + ../src/libvirt_util.la \ + $(qemu_LDADDS) \ + ../gnulib/lib/libgnu.la \ + $(NULL) + domainsnapshotxml2xmltest_SOURCES = \ domainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \ testutils.c testutils.h @@ -686,6 +697,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \ qemuagenttest.c qemucapabilitiestest.c \ qemucaps2xmltest.c qemucommandutiltest.c \ qemumemlocktest.c qemucpumock.c testutilshostcpus.h \ + qemublocktest.c \ $(QEMUMONITORTESTUTILS_SOURCES) endif ! WITH_QEMU diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c new file mode 100644 index 000000000..52ecbde36 --- /dev/null +++ b/tests/qemublocktest.c @@ -0,0 +1,189 @@ +/* + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>;. + */ + +#include <config.h> + +#include <stdlib.h> + +#include "testutils.h" +#include "virstoragefile.h" +#include "virstring.h" +#include "virlog.h" +#include "qemu/qemu_block.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +VIR_LOG_INIT("tests.storagetest"); + +struct testBackingXMLjsonXMLdata { + int type; + const char *xml; +}; + +static int +testBackingXMLjsonXML(const void *args) +{ + const struct testBackingXMLjsonXMLdata *data = args; + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + virStorageSourcePtr xmlsrc = NULL; + virStorageSourcePtr jsonsrc = NULL; + virJSONValuePtr backendprops = NULL; + virJSONValuePtr wrapper = NULL; + char *propsstr = NULL; + char *protocolwrapper = NULL; + char *actualxml = NULL; + int ret = -1; + + if (VIR_ALLOC(xmlsrc) < 0) + return -1; + + xmlsrc->type = data->type; + + if (!(xml = virXMLParseStringCtxt(data->xml, "(test storage source XML)", &ctxt))) + goto cleanup; + + if (virDomainDiskSourceParse(ctxt->node, ctxt, xmlsrc, 0) < 0) { + fprintf(stderr, "failed to parse disk source xml\n"); + goto cleanup; + } + + if (!(backendprops = qemuBlockStorageSourceGetBackendProps(xmlsrc))) { + fprintf(stderr, "failed to format disk source json\n"); + goto cleanup; + } + + if (virJSONValueObjectCreate(&wrapper, "a:file", backendprops, NULL) < 0) + goto cleanup; + + backendprops = NULL; + + if (!(propsstr = virJSONValueToString(wrapper, false))) + goto cleanup; + + if (virAsprintf(&protocolwrapper, "json:%s", propsstr) < 0) + goto cleanup; + + if (!(jsonsrc = virStorageSourceNewFromBackingAbsolute(protocolwrapper))) { + fprintf(stderr, "failed to parse disk json\n"); + goto cleanup; + } + + if (virDomainDiskSourceFormat(&buf, jsonsrc, 0, 0) < 0 || + !(actualxml = virBufferContentAndReset(&buf))) { + fprintf(stderr, "failed to format disk source xml\n"); + goto cleanup; + } + + if (STRNEQ(actualxml, data->xml)) { + fprintf(stderr, "\n expected storage source xml:\n'%s'\n" + "actual storage source xml:\n%s\n" + "intermediate json:\n%s\n", + data->xml, actualxml, protocolwrapper); + goto cleanup; + } + + ret = 0; + + cleanup: + virStorageSourceFree(xmlsrc); + virStorageSourceFree(jsonsrc); + VIR_FREE(propsstr); + VIR_FREE(protocolwrapper); + VIR_FREE(actualxml); + virJSONValueFree(backendprops); + virJSONValueFree(wrapper); + virBufferFreeAndReset(&buf); + xmlFreeDoc(xml); + + return ret; +} + + +static int +mymain(void) +{ + int ret = 0; + struct testBackingXMLjsonXMLdata data; + + virTestCounterReset("qemu storage source xml->json->xml "); + +#define TEST_JSON_FORMAT(tpe, xmlstr) \ + do { \ + data.type = tpe; \ + data.xml = xmlstr; \ + if (virTestRun(virTestCounterNext(), testBackingXMLjsonXML, &data) < 0) \ + ret = -1; \ + } while (0) + +#define TEST_JSON_FORMAT_NET(xmlstr)\ + TEST_JSON_FORMAT(VIR_STORAGE_TYPE_NETWORK, xmlstr) + + TEST_JSON_FORMAT(VIR_STORAGE_TYPE_FILE, "<source file='/path/to/file'/>\n"); + + /* type VIR_STORAGE_TYPE_BLOCK is not tested since it parses back to 'file' */ + /* type VIR_STORAGE_TYPE_DIR it is a 'format' driver in qemu */ + + TEST_JSON_FORMAT_NET("<source protocol='http' name='file'>\n" + " <host name='example.com' port='80'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='https' name='file'>\n" + " <host name='example.com' port='432'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='gluster' name='vol/file'>\n" + " <host name='example.com' port='24007'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='gluster' name='testvol/img.qcow2'>\n" + " <host name='example.com' port='1234'/>\n" + " <host transport='unix' socket='/path/socket'/>\n" + " <host name='example.com' port='24007'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='nbd'>\n" + " <host transport='unix' socket='/path/to/socket'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='nbd' name='blah'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='nbd'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='ssh' name='blah'>\n" + " <host name='example.org' port='6000'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='rbd' name='libvirt/test'>\n" + " <host name='example.com' port='1234'/>\n" + " <host name='example2.com'/>\n" + " <snapshot name='snapshotname'/>\n" + " <config file='/path/to/conf'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/0'>\n" + " <host name='test.org' port='3260'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/6'>\n" + " <host name='test.org' port='1234'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='sheepdog' name='test'>\n" + " <host name='example.com' port='321'/>\n" + "</source>\n"); + TEST_JSON_FORMAT_NET("<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='9999'/>\n" + "</source>\n"); + + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; +} + +VIR_TEST_MAIN(mymain) -- 2.14.3