[libvirt] [PATCH 00/10] qemu: clean up formatting of networked disk sources
Few cleanups I made to cleanup the def->JSON generator for qemu disk sources. Peter Krempa (10): qemu: command: Set port number only for TCP transport util: Extract helper to retrieve default port for network protocol util: storage: Fill in default ports for gluster and iscsi conf: Pre-fill default ports when parsing network disk sources qemu: command: Remove default port numbers for NBD and GLUSTER qemu: command: Call qemuGetDriveSourceProps only if necessary qemu: Move qemuGetDriveSourceProps to qemu_block qemu: block: Refactor and rename qemuGetDriveSourceProps qemu: block: refactor and rename qemuBuildGlusterDriveJSONHosts qemu: block: rename and refactor qemuBuildGlusterDriveJSON po/POTFILES.in | 1 + src/conf/domain_conf.c | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_block.c | 156 ++++++++++++++ src/qemu/qemu_block.h | 3 + src/qemu/qemu_command.c | 229 ++++----------------- src/util/virstoragefile.c | 65 ++++++ src/util/virstoragefile.h | 4 + .../generic-disk-network-http.xml | 4 +- .../qemuxml2xmlout-disk-drive-network-gluster.xml | 2 +- 10 files changed, 273 insertions(+), 195 deletions(-) -- 2.12.2
Setting port number for protocols using UNIX transport does not make sense. Move the setter code to the appropriate block. --- src/qemu/qemu_command.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3fe291863..f77ae91c3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -949,6 +949,10 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, goto cleanup; if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, + src->hosts->port)) < 0) + goto cleanup; + if (VIR_STRDUP(uri->scheme, virStorageNetProtocolTypeToString(src->protocol)) < 0) goto cleanup; @@ -959,10 +963,6 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, goto cleanup; } - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, - src->hosts->port)) < 0) - goto cleanup; - if (src->path) { if (src->volume) { if (virAsprintf(&uri->path, "/%s%s", -- 2.12.2
Make the stuff hardcoded in qemu a global helper so that other parts of the code can determine the default port too. --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 55 ++++++++--------------------------------------- src/util/virstoragefile.c | 43 ++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 +++ 4 files changed, 56 insertions(+), 46 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 888412ac7..44b5dc1e0 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2614,6 +2614,7 @@ virStorageSourceIsBlockLocal; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceIsRelative; +virStorageSourceNetworkDefaultPort; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; virStorageSourceParseRBDColonString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f77ae91c3..e5f4bdab7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -482,55 +482,18 @@ qemuNetworkDriveGetPort(int protocol, { int ret = 0; - if (port) { - if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse port number '%s'"), - port); - return -1; - } - - return ret; - } - - switch ((virStorageNetProtocol) protocol) { - case VIR_STORAGE_NET_PROTOCOL_HTTP: - return 80; - - case VIR_STORAGE_NET_PROTOCOL_HTTPS: - return 443; - - case VIR_STORAGE_NET_PROTOCOL_FTP: - return 21; - - case VIR_STORAGE_NET_PROTOCOL_FTPS: - return 990; - - case VIR_STORAGE_NET_PROTOCOL_TFTP: - return 69; - - case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: - return 7000; - - case VIR_STORAGE_NET_PROTOCOL_NBD: - return 10809; - - case VIR_STORAGE_NET_PROTOCOL_SSH: - return 22; - - case VIR_STORAGE_NET_PROTOCOL_ISCSI: - case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - /* no default port specified */ - return 0; + if (!port && + !(port = virStorageSourceNetworkDefaultPort(protocol))) + return -1; - case VIR_STORAGE_NET_PROTOCOL_RBD: - case VIR_STORAGE_NET_PROTOCOL_LAST: - case VIR_STORAGE_NET_PROTOCOL_NONE: - /* not applicable */ - return -1; + if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to parse port number '%s'"), + port); + return -1; } - return -1; + return ret; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f0ed5c6bd..cae20eccc 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4018,3 +4018,46 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top, *idx = 0; return NULL; } + + +const char * +virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) +{ + switch (protocol) { + case VIR_STORAGE_NET_PROTOCOL_HTTP: + return "80"; + + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + return "443"; + + case VIR_STORAGE_NET_PROTOCOL_FTP: + return "21"; + + case VIR_STORAGE_NET_PROTOCOL_FTPS: + return "990"; + + case VIR_STORAGE_NET_PROTOCOL_TFTP: + return "69"; + + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + return "7000"; + + case VIR_STORAGE_NET_PROTOCOL_NBD: + return "10809"; + + case VIR_STORAGE_NET_PROTOCOL_SSH: + return "22"; + + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: + /* no default port specified */ + return "0"; + + case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_LAST: + case VIR_STORAGE_NET_PROTOCOL_NONE: + return NULL; + } + + return NULL; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 0bff8671f..877c3af4d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -406,4 +406,7 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top, unsigned int *index) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +const char * +virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol); + #endif /* __VIR_STORAGE_FILE_H__ */ -- 2.12.2
Our documentation provides them, so the helper should return them. --- src/util/virstoragefile.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cae20eccc..0c215e1b6 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4049,11 +4049,15 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) return "22"; case VIR_STORAGE_NET_PROTOCOL_ISCSI: + return "3260"; + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - /* no default port specified */ - return "0"; + return "24007"; case VIR_STORAGE_NET_PROTOCOL_RBD: + /* we don't provide a default for RBD */ + return NULL; + case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: return NULL; -- 2.12.2
Fill them in right away rather than having to figure out at runtime whether they are necessary or not. virStorageSourceNetworkDefaultPort does not need to be exported any more. --- src/conf/domain_conf.c | 3 +++ src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 10 ++-------- src/util/virstoragefile.c | 20 +++++++++++++++++++- src/util/virstoragefile.h | 5 +++-- .../generic-disk-network-http.xml | 4 ++-- .../qemuxml2xmlout-disk-drive-network-gluster.xml | 2 +- 7 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c3149f976..bc4b10d60 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7807,6 +7807,9 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainStorageHostParse(node, &src->hosts, &src->nhosts) < 0) goto cleanup; + + if (virStorageSourceNetworkAssignDefaultPorts(src) < 0) + goto cleanup; break; case VIR_STORAGE_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(node, &src->srcpool) < 0) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 44b5dc1e0..ae8d3483c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2614,7 +2614,7 @@ virStorageSourceIsBlockLocal; virStorageSourceIsEmpty; virStorageSourceIsLocalStorage; virStorageSourceIsRelative; -virStorageSourceNetworkDefaultPort; +virStorageSourceNetworkAssignDefaultPorts; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; virStorageSourceParseRBDColonString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e5f4bdab7..285167145 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -477,15 +477,10 @@ qemuSafeSerialParamValue(const char *value) static int -qemuNetworkDriveGetPort(int protocol, - const char *port) +qemuNetworkDriveGetPort(const char *port) { int ret = 0; - if (!port && - !(port = virStorageSourceNetworkDefaultPort(protocol))) - return -1; - if (virStrToLong_i(port, NULL, 10, &ret) < 0 || ret < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), @@ -912,8 +907,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, goto cleanup; if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, - src->hosts->port)) < 0) + if ((uri->port = qemuNetworkDriveGetPort(src->hosts->port)) < 0) goto cleanup; if (VIR_STRDUP(uri->scheme, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0c215e1b6..6c2516d34 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4020,7 +4020,7 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top, } -const char * +static const char * virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) { switch (protocol) { @@ -4065,3 +4065,21 @@ virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol) return NULL; } + + +int +virStorageSourceNetworkAssignDefaultPorts(virStorageSourcePtr src) +{ + size_t i; + + for (i = 0; i < src->nhosts; i++) { + if (src->hosts[i].transport == VIR_STORAGE_NET_HOST_TRANS_TCP && + src->hosts[i].port == NULL) { + if (VIR_STRDUP(src->hosts[i].port, + virStorageSourceNetworkDefaultPort(src->protocol)) < 0) + return -1; + } + } + + return 0; +} diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 877c3af4d..98992e04a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -406,7 +406,8 @@ virStorageSourceFindByNodeName(virStorageSourcePtr top, unsigned int *index) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -const char * -virStorageSourceNetworkDefaultPort(virStorageNetProtocol protocol); +int +virStorageSourceNetworkAssignDefaultPorts(virStorageSourcePtr src) + ATTRIBUTE_NONNULL(1); #endif /* __VIR_STORAGE_FILE_H__ */ diff --git a/tests/genericxml2xmlindata/generic-disk-network-http.xml b/tests/genericxml2xmlindata/generic-disk-network-http.xml index 51c779502..ec4520c7b 100644 --- a/tests/genericxml2xmlindata/generic-disk-network-http.xml +++ b/tests/genericxml2xmlindata/generic-disk-network-http.xml @@ -17,14 +17,14 @@ <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='http' name='test.img'> - <host name='example.org'/> + <host name='example.org' port='80'/> </source> <target dev='vda' bus='virtio'/> </disk> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> <source protocol='https' name='test2.img'> - <host name='example.org'/> + <host name='example.org' port='443'/> </source> <target dev='vdb' bus='virtio'/> </disk> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-gluster.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-gluster.xml index 66a84750a..2c7c46b01 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-gluster.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-gluster.xml @@ -34,7 +34,7 @@ <driver name='qemu' type='qcow2'/> <source protocol='gluster' name='Volume3/Image.qcow2'> <host name='example.org' port='6000'/> - <host name='example.org'/> + <host name='example.org' port='24007'/> <host transport='unix' socket='/path/to/sock'/> </source> <target dev='vdc' bus='virtio'/> -- 2.12.2
The command line generators for the protocols above hardcoded a default port number. Since we now always assign it when parsing the source definition, this ad-hoc code is not required any more. --- src/qemu/qemu_command.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 285167145..10c2dea93 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -792,9 +792,6 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, } -#define QEMU_DEFAULT_NBD_PORT "10809" -#define QEMU_DEFAULT_GLUSTER_PORT "24007" - /* builds the hosts array */ static virJSONValuePtr qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) @@ -804,7 +801,6 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) virJSONValuePtr ret = NULL; virStorageNetHostDefPtr host; const char *transport; - const char *portstr; size_t i; if (!(servers = virJSONValueNewArray())) @@ -813,19 +809,15 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) for (i = 0; i < src->nhosts; i++) { host = src->hosts + i; transport = virStorageNetHostTransportTypeToString(host->transport); - portstr = host->port; if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0) goto cleanup; - if (!portstr) - portstr = QEMU_DEFAULT_GLUSTER_PORT; - switch ((virStorageNetHostTransport) host->transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: if (virJSONValueObjectAdd(server, "s:host", host->name, - "s:port", portstr, + "s:port", host->port, NULL) < 0) goto cleanup; break; @@ -979,10 +971,8 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, switch (src->hosts->transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: - virBufferStrcat(&buf, src->hosts->name, NULL); - virBufferAsprintf(&buf, ":%s", - src->hosts->port ? src->hosts->port : - QEMU_DEFAULT_NBD_PORT); + virBufferStrcat(&buf, src->hosts->name, ":", + src->hosts->port, NULL); break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: -- 2.12.2
Add logic which will call qemuGetDriveSourceProps only in cases where we need the JSON representation. This will allow qemuGetDriveSourceProps to generate the JSON representation for all possible disk sources. --- src/qemu/qemu_command.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 10c2dea93..376082320 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1436,6 +1436,28 @@ qemuDiskBusNeedsDeviceArg(int bus) } +/** + * qemuDiskSourceNeedsProps: + * @src: disk source + * + * Returns true, if the disk source needs to be generated from the JSON + * representation. Otherwise, the disk source should be represented using + * the legacy representation. + */ +static bool +qemuDiskSourceNeedsProps(virStorageSourcePtr src) +{ + int actualType = virStorageSourceGetActualType(src); + + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER && + src->nhosts > 1) + return true; + + return false; +} + + static int qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virQEMUDriverConfigPtr cfg, @@ -1450,7 +1472,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, char *source = NULL; int ret = -1; - if (qemuGetDriveSourceProps(disk->src, &srcprops) < 0) + if (qemuDiskSourceNeedsProps(disk->src) && + qemuGetDriveSourceProps(disk->src, &srcprops) < 0) goto cleanup; if (!srcprops && -- 2.12.2
Pure code movement except for the tweaks necessary for cross-usage. --- po/POTFILES.in | 1 + src/qemu/qemu_block.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 5 ++ src/qemu/qemu_command.c | 127 +----------------------------------------------- 4 files changed, 133 insertions(+), 126 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 275df1f29..b5e99e084 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -129,6 +129,7 @@ src/openvz/openvz_util.c src/phyp/phyp_driver.c src/qemu/qemu_agent.c src/qemu/qemu_alias.c +src/qemu/qemu_block.c src/qemu/qemu_capabilities.c src/qemu/qemu_cgroup.c src/qemu/qemu_command.c diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 29b5c4756..e6b909015 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -428,3 +428,129 @@ qemuBlockGetNodeData(virJSONValuePtr data) virHashFree(ret); return NULL; } + + +/* builds the hosts array */ +static virJSONValuePtr +qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) +{ + virJSONValuePtr servers = NULL; + virJSONValuePtr server = NULL; + virJSONValuePtr ret = NULL; + virStorageNetHostDefPtr host; + const char *transport; + size_t i; + + if (!(servers = virJSONValueNewArray())) + goto cleanup; + + for (i = 0; i < src->nhosts; i++) { + host = src->hosts + i; + transport = virStorageNetHostTransportTypeToString(host->transport); + + if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0) + goto cleanup; + + switch ((virStorageNetHostTransport) host->transport) { + case VIR_STORAGE_NET_HOST_TRANS_TCP: + if (virJSONValueObjectAdd(server, + "s:host", host->name, + "s:port", host->port, + NULL) < 0) + goto cleanup; + break; + + case VIR_STORAGE_NET_HOST_TRANS_UNIX: + if (virJSONValueObjectAdd(server, + "s:socket", host->socket, + NULL) < 0) + goto cleanup; + break; + + case VIR_STORAGE_NET_HOST_TRANS_RDMA: + case VIR_STORAGE_NET_HOST_TRANS_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("transport protocol '%s' is not yet supported"), + transport); + goto cleanup; + } + + if (virJSONValueArrayAppend(servers, server) < 0) + goto cleanup; + + server = NULL; + } + + ret = servers; + servers = NULL; + + cleanup: + virJSONValueFree(servers); + virJSONValueFree(server); + + return ret; +} + + +static virJSONValuePtr +qemuBuildGlusterDriveJSON(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + virJSONValuePtr servers = NULL; + virJSONValuePtr ret = NULL; + + if (!(servers = qemuBuildGlusterDriveJSONHosts(src))) + return NULL; + + /* { driver:"gluster", + * volume:"testvol", + * path:"/a.img", + * server :[{type:"tcp", host:"1.2.3.4", port:24007}, + * {type:"unix", socket:"/tmp/glusterd.socket"}, ...]} + */ + if (virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:volume", src->volume, + "s:path", src->path, + "a:server", servers, NULL) < 0) + virJSONValueFree(servers); + + return ret; +} + + +int +qemuGetDriveSourceProps(virStorageSourcePtr src, + virJSONValuePtr *props) +{ + int actualType = virStorageSourceGetActualType(src); + virJSONValuePtr fileprops = NULL; + + *props = NULL; + + switch ((virStorageType) actualType) { + case VIR_STORAGE_TYPE_BLOCK: + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_DIR: + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: + break; + + case VIR_STORAGE_TYPE_NETWORK: + if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER && + src->nhosts > 1) { + if (!(fileprops = qemuBuildGlusterDriveJSON(src))) + return -1; + } + break; + } + + if (fileprops && + virJSONValueObjectCreate(props, "a:file", fileprops, NULL) < 0) { + virJSONValueFree(fileprops); + return -1; + } + + return 0; +} diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 2af15a65a..3a8950b13 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -53,4 +53,9 @@ qemuBlockNodeNamesDetect(virQEMUDriverPtr driver, virHashTablePtr qemuBlockGetNodeData(virJSONValuePtr data); + +int +qemuGetDriveSourceProps(virStorageSourcePtr src, + virJSONValuePtr *props); + #endif /* __QEMU_BLOCK_H__ */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 376082320..963224335 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -29,6 +29,7 @@ #include "qemu_interface.h" #include "qemu_alias.h" #include "qemu_security.h" +#include "qemu_block.h" #include "cpu/cpu.h" #include "dirname.h" #include "viralloc.h" @@ -792,95 +793,6 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd, } -/* builds the hosts array */ -static virJSONValuePtr -qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) -{ - virJSONValuePtr servers = NULL; - virJSONValuePtr server = NULL; - virJSONValuePtr ret = NULL; - virStorageNetHostDefPtr host; - const char *transport; - size_t i; - - if (!(servers = virJSONValueNewArray())) - goto cleanup; - - for (i = 0; i < src->nhosts; i++) { - host = src->hosts + i; - transport = virStorageNetHostTransportTypeToString(host->transport); - - if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0) - goto cleanup; - - switch ((virStorageNetHostTransport) host->transport) { - case VIR_STORAGE_NET_HOST_TRANS_TCP: - if (virJSONValueObjectAdd(server, - "s:host", host->name, - "s:port", host->port, - NULL) < 0) - goto cleanup; - break; - - case VIR_STORAGE_NET_HOST_TRANS_UNIX: - if (virJSONValueObjectAdd(server, - "s:socket", host->socket, - NULL) < 0) - goto cleanup; - break; - - case VIR_STORAGE_NET_HOST_TRANS_RDMA: - case VIR_STORAGE_NET_HOST_TRANS_LAST: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("transport protocol '%s' is not yet supported"), - transport); - goto cleanup; - } - - if (virJSONValueArrayAppend(servers, server) < 0) - goto cleanup; - - server = NULL; - } - - ret = servers; - servers = NULL; - - cleanup: - virJSONValueFree(servers); - virJSONValueFree(server); - - return ret; -} - - -static virJSONValuePtr -qemuBuildGlusterDriveJSON(virStorageSourcePtr src) -{ - const char *protocol = virStorageNetProtocolTypeToString(src->protocol); - virJSONValuePtr servers = NULL; - virJSONValuePtr ret = NULL; - - if (!(servers = qemuBuildGlusterDriveJSONHosts(src))) - return NULL; - - /* { driver:"gluster", - * volume:"testvol", - * path:"/a.img", - * server :[{type:"tcp", host:"1.2.3.4", port:24007}, - * {type:"unix", socket:"/tmp/glusterd.socket"}, ...]} - */ - if (virJSONValueObjectCreate(&ret, - "s:driver", protocol, - "s:volume", src->volume, - "s:path", src->path, - "a:server", servers, NULL) < 0) - virJSONValueFree(servers); - - return ret; -} - - static char * qemuBuildNetworkDriveURI(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo) @@ -1103,43 +1015,6 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, } -static int -qemuGetDriveSourceProps(virStorageSourcePtr src, - virJSONValuePtr *props) -{ - int actualType = virStorageSourceGetActualType(src); - virJSONValuePtr fileprops = NULL; - - *props = NULL; - - switch ((virStorageType) actualType) { - case VIR_STORAGE_TYPE_BLOCK: - case VIR_STORAGE_TYPE_FILE: - case VIR_STORAGE_TYPE_DIR: - case VIR_STORAGE_TYPE_VOLUME: - case VIR_STORAGE_TYPE_NONE: - case VIR_STORAGE_TYPE_LAST: - break; - - case VIR_STORAGE_TYPE_NETWORK: - if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER && - src->nhosts > 1) { - if (!(fileprops = qemuBuildGlusterDriveJSON(src))) - return -1; - } - break; - } - - if (fileprops && - virJSONValueObjectCreate(props, "a:file", fileprops, NULL) < 0) { - virJSONValueFree(fileprops); - return -1; - } - - return 0; -} - - int qemuGetDriveSourceString(virStorageSourcePtr src, qemuDomainSecretInfoPtr secinfo, -- 2.12.2
Rename it to qemuBlockStorageSourceGetBackendProps and refactor it to return the JSON object instead of filling a pointer since now it's always expected to return data. --- src/qemu/qemu_block.c | 49 +++++++++++++++++++++++++++++++++++-------------- src/qemu/qemu_block.h | 6 ++---- src/qemu/qemu_command.c | 2 +- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index e6b909015..3dbb5586d 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -519,14 +519,19 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) } -int -qemuGetDriveSourceProps(virStorageSourcePtr src, - virJSONValuePtr *props) +/** + * qemuBlockStorageSourceGetBackendProps: + * @src: disk source + * + * Creates a JSON object describing the underlying storage or protocol of a + * storage source. Returns NULL on error and reports an appropriate error message. + */ +virJSONValuePtr +qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src) { int actualType = virStorageSourceGetActualType(src); virJSONValuePtr fileprops = NULL; - - *props = NULL; + virJSONValuePtr ret = NULL; switch ((virStorageType) actualType) { case VIR_STORAGE_TYPE_BLOCK: @@ -538,19 +543,35 @@ qemuGetDriveSourceProps(virStorageSourcePtr src, break; case VIR_STORAGE_TYPE_NETWORK: - if (src->protocol == VIR_STORAGE_NET_PROTOCOL_GLUSTER && - src->nhosts > 1) { + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_GLUSTER: if (!(fileprops = qemuBuildGlusterDriveJSON(src))) - return -1; + goto cleanup; + break; + + case VIR_STORAGE_NET_PROTOCOL_NBD: + case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + case VIR_STORAGE_NET_PROTOCOL_HTTP: + case VIR_STORAGE_NET_PROTOCOL_HTTPS: + case VIR_STORAGE_NET_PROTOCOL_FTP: + case VIR_STORAGE_NET_PROTOCOL_FTPS: + case VIR_STORAGE_NET_PROTOCOL_TFTP: + case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_NONE: + case VIR_STORAGE_NET_PROTOCOL_LAST: + break; } break; } - if (fileprops && - virJSONValueObjectCreate(props, "a:file", fileprops, NULL) < 0) { - virJSONValueFree(fileprops); - return -1; - } + if (virJSONValueObjectCreate(&ret, "a:file", fileprops, NULL) < 0) + goto cleanup; - return 0; + fileprops = NULL; + + cleanup: + virJSONValueFree(fileprops); + return ret; } diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index 3a8950b13..17dec799f 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -53,9 +53,7 @@ qemuBlockNodeNamesDetect(virQEMUDriverPtr driver, virHashTablePtr qemuBlockGetNodeData(virJSONValuePtr data); - -int -qemuGetDriveSourceProps(virStorageSourcePtr src, - virJSONValuePtr *props); +virJSONValuePtr +qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src); #endif /* __QEMU_BLOCK_H__ */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 963224335..c20dd64dc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1348,7 +1348,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, int ret = -1; if (qemuDiskSourceNeedsProps(disk->src) && - qemuGetDriveSourceProps(disk->src, &srcprops) < 0) + !(srcprops = qemuBlockStorageSourceGetBackendProps(disk->src))) goto cleanup; if (!srcprops && -- 2.12.2
New name is qemuBlockStorageSourceBuildHostsJSONSocketAddress since it formats the JSON object in accordance with qemu's SocketAddress type. Since the new naming in qemu uses 'inet' instead of 'tcp' add a compatibility layer for gluster which uses the old name. --- src/qemu/qemu_block.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 3dbb5586d..73f209060 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -430,9 +430,17 @@ qemuBlockGetNodeData(virJSONValuePtr data) } -/* builds the hosts array */ +/** + * qemuBlockStorageSourceBuildHostsJSONSocketAddress: + * @src: disk storage source + * @legacy: use 'tcp' instead of 'inet' for compatibility reasons + * + * Formats src->hosts into a json object conforming to the 'SocketAddress' type + * in qemu. + */ static virJSONValuePtr -qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) +qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src, + bool legacy) { virJSONValuePtr servers = NULL; virJSONValuePtr server = NULL; @@ -446,24 +454,27 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) for (i = 0; i < src->nhosts; i++) { host = src->hosts + i; - transport = virStorageNetHostTransportTypeToString(host->transport); - - if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0) - goto cleanup; switch ((virStorageNetHostTransport) host->transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: - if (virJSONValueObjectAdd(server, - "s:host", host->name, - "s:port", host->port, - NULL) < 0) + if (legacy) + transport = "tcp"; + else + transport = "inet"; + + if (virJSONValueObjectCreate(&server, + "s:type", transport, + "s:host", host->name, + "s:port", host->port, + NULL) < 0) goto cleanup; break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: - if (virJSONValueObjectAdd(server, - "s:socket", host->socket, - NULL) < 0) + if (virJSONValueObjectCreate(&server, + "s:type", "unix", + "s:socket", host->socket, + NULL) < 0) goto cleanup; break; @@ -481,8 +492,7 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) server = NULL; } - ret = servers; - servers = NULL; + VIR_STEAL_PTR(ret, servers); cleanup: virJSONValueFree(servers); @@ -499,7 +509,7 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) virJSONValuePtr servers = NULL; virJSONValuePtr ret = NULL; - if (!(servers = qemuBuildGlusterDriveJSONHosts(src))) + if (!(servers = qemuBlockStorageSourceBuildHostsJSONSocketAddress(src, true))) return NULL; /* { driver:"gluster", -- 2.12.2
New name is qemuBlockStorageSourceGetGlusterProps and also hardcode the protocol name rather than calling the ToString function, since this function can't be made universal. --- src/qemu/qemu_block.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 73f209060..ccaf32611 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -503,9 +503,8 @@ qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src, static virJSONValuePtr -qemuBuildGlusterDriveJSON(virStorageSourcePtr src) +qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) { - const char *protocol = virStorageNetProtocolTypeToString(src->protocol); virJSONValuePtr servers = NULL; virJSONValuePtr ret = NULL; @@ -519,7 +518,7 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) * {type:"unix", socket:"/tmp/glusterd.socket"}, ...]} */ if (virJSONValueObjectCreate(&ret, - "s:driver", protocol, + "s:driver", "gluster", "s:volume", src->volume, "s:path", src->path, "a:server", servers, NULL) < 0) @@ -555,7 +554,7 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src) case VIR_STORAGE_TYPE_NETWORK: switch ((virStorageNetProtocol) src->protocol) { case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - if (!(fileprops = qemuBuildGlusterDriveJSON(src))) + if (!(fileprops = qemuBlockStorageSourceGetGlusterProps(src))) goto cleanup; break; -- 2.12.2
On 07/10/2017 02:07 PM, Peter Krempa wrote:
Few cleanups I made to cleanup the def->JSON generator for qemu disk sources.
Peter Krempa (10): qemu: command: Set port number only for TCP transport util: Extract helper to retrieve default port for network protocol util: storage: Fill in default ports for gluster and iscsi conf: Pre-fill default ports when parsing network disk sources qemu: command: Remove default port numbers for NBD and GLUSTER qemu: command: Call qemuGetDriveSourceProps only if necessary qemu: Move qemuGetDriveSourceProps to qemu_block qemu: block: Refactor and rename qemuGetDriveSourceProps qemu: block: refactor and rename qemuBuildGlusterDriveJSONHosts qemu: block: rename and refactor qemuBuildGlusterDriveJSON
po/POTFILES.in | 1 + src/conf/domain_conf.c | 3 + src/libvirt_private.syms | 1 + src/qemu/qemu_block.c | 156 ++++++++++++++ src/qemu/qemu_block.h | 3 + src/qemu/qemu_command.c | 229 ++++----------------- src/util/virstoragefile.c | 65 ++++++ src/util/virstoragefile.h | 4 + .../generic-disk-network-http.xml | 4 +- .../qemuxml2xmlout-disk-drive-network-gluster.xml | 2 +- 10 files changed, 273 insertions(+), 195 deletions(-)
ACK series. Although, for future work it might be interesting to look at possibility of storing port as an int instead of string? Michal
participants (2)
-
Michal Privoznik -
Peter Krempa