On Tue, Dec 06, 2016 at 22:51:58 +0530, Prasanna Kumar Kalever wrote:
> Currently, the Host object looks like
>
> struct _virStorageNetHostDef {
> char *name;
> char *port;
> int transport; /* virStorageNetHostTransport */
> char *socket; /* path to unix socket */
> }
>
> We don't actually need a 'name' and 'port' if the transport type is unix
> domain sockets, and if the transport is inet tcp/rdma we don't actually
> care for socket path.
>
> With a simple union discrimination we can make this much better
>
> struct _virStorageNetHostUnixSockAddr {
> char *path;
> };
>
> struct _virStorageNetHostInetSockAddr {
> char *addr;
> char *port;
> };
>
> struct _virStorageNetHostDef {
> virStorageNetHostTransport type;
> union { /* union tag is @type */
> virStorageNetHostUnixSockAddr uds;
> virStorageNetHostInetSockAddr inet;
> } u;
> };
I'm not really persuaded that this is much better than the current
state. Data-wise you save one char *. Code wise you add the complexity
of accessing the enum everywhere.
>
> This patch performs the required changes in transforming _virStorageNetHostDef
> to fit union discrimination.
On a machine with xen installed this fails to compile:
xenconfig/xen_xl.c: In function 'xenFormatXLDiskSrcNet':
xenconfig/xen_xl.c:954:41: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
if (strchr(src->hosts[i].name, ':'))
^
xenconfig/xen_xl.c:956:50: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
src->hosts[i].name);
^
xenconfig/xen_xl.c:958:64: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'name'
virBufferAsprintf(&buf, "%s", src->hosts[i].name);
^
xenconfig/xen_xl.c:960:34: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port'
if (src->hosts[i].port)
^
xenconfig/xen_xl.c:961:69: error: 'virStorageNetHostDef {aka struct _virStorageNetHostDef}' has no member named 'port'
virBufferAsprintf(&buf, "\\\\:%s", src->hosts[i].port);
I suspect there are more than just these in other hypervisor drivers.
As said. I'm not a fan of this change. If you want to continue doing
this, please make sure that you install all VM drivers so that you
change all the appropriate places.
Also please make sure to use a switch statement in appropriate places as
described below.
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 52 +++++++++++----------
> src/qemu/qemu_command.c | 68 ++++++++++++++--------------
> src/qemu/qemu_parse_command.c | 44 +++++++++---------
> src/storage/storage_backend_gluster.c | 21 ++++-----
> src/storage/storage_driver.c | 7 ++-
> src/util/virstoragefile.c | 85 ++++++++++++++++++-----------------
> src/util/virstoragefile.h | 20 +++++++--
> tests/virstoragetest.c | 2 +-
> 8 files changed, 160 insertions(+), 139 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 0e879e1..20d4d01 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5967,6 +5967,7 @@ virDomainStorageHostParse(xmlNodePtr node,
> int ret = -1;
> xmlNodePtr child;
> char *transport = NULL;
> + char *socket = NULL;
> virStorageNetHostDef host;
>
> memset(&host, 0, sizeof(host));
> @@ -5976,12 +5977,13 @@ virDomainStorageHostParse(xmlNodePtr node,
> if (child->type == XML_ELEMENT_NODE &&
> xmlStrEqual(child->name, BAD_CAST "host")) {
>
> - host.transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
> + host.type = VIR_STORAGE_NET_HOST_TRANS_TCP;
>
> /* transport can be tcp (default), unix or rdma. */
> if ((transport = virXMLPropString(child, "transport"))) {
> - host.transport = virStorageNetHostTransportTypeFromString(transport);
> - if (host.transport < 0) {
> + host.type = virStorageNetHostTransportTypeFromString(transport);
> + if ((host.type < 0) ||
> + (host.type >= VIR_STORAGE_NET_HOST_TRANS_LAST)) {
Too many parentheses, the inner terms have the same operator priority as
with the parentheses.
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("unknown protocol transport type '%s'"),
> transport);
[...]
> @@ -20322,17 +20325,20 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf,
>
> for (n = 0; n < src->nhosts; n++) {
> virBufferAddLit(buf, "<host");
> - virBufferEscapeString(buf, " name='%s'",
> - src->hosts[n].name);
> - virBufferEscapeString(buf, " port='%s'",
> - src->hosts[n].port);
> + if (src->hosts[n].type != VIR_STORAGE_NET_HOST_TRANS_UNIX) {
> + virBufferEscapeString(buf, " name='%s'",
> + src->hosts[n].u.inet.addr);
> + virBufferEscapeString(buf, " port='%s'",
> + src->hosts[n].u.inet.port);
> + }
>
> - if (src->hosts[n].transport)
> + if (src->hosts[n].type)
> virBufferAsprintf(buf, " transport='%s'",
> - virStorageNetHostTransportTypeToString(src->hosts[n].transport));
> + virStorageNetHostTransportTypeToString(src->hosts[n].type));
>
> - virBufferEscapeString(buf, " socket='%s'",
> - src->hosts[n].socket);
> + if (src->hosts[n].type == VIR_STORAGE_NET_HOST_TRANS_UNIX)
> + virBufferEscapeString(buf, " socket='%s'",
> + src->hosts[n].u.uds.path);
>
> virBufferAddLit(buf, "/>\n");
You need to convert this hunk to use a switch, so the code is
extensible.
> }
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 6c457dd..fd62597 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -822,19 +822,18 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src)
>
> for (i = 0; i < src->nhosts; i++) {
> host = src->hosts + i;
> - transport = virStorageNetHostTransportTypeToString(host->transport);
> - portstr = host->port;
> + transport = virStorageNetHostTransportTypeToString(host->type);
>
> if (virJSONValueObjectCreate(&server, "s:type", transport, NULL) < 0)
> goto cleanup;
>
> - if (!portstr)
> - portstr = QEMU_DEFAULT_GLUSTER_PORT;
> -
> - switch ((virStorageNetHostTransport) host->transport) {
> + switch ((virStorageNetHostTransport) host->type) {
> case VIR_STORAGE_NET_HOST_TRANS_TCP:
> + portstr = host->u.inet.port;
> + if (!portstr)
> + portstr = QEMU_DEFAULT_GLUSTER_PORT;
Wrong indentation.
> if (virJSONValueObjectAdd(server,
> - "s:host", host->name,
> + "s:host", host->u.inet.addr,
> "s:port", portstr,
> NULL) < 0)
> goto cleanup;
[...]
> @@ -944,15 +943,16 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> }
> }
>
> - if (src->hosts->socket &&
> - virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0)
> + if ((src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) &&
> + (virAsprintf(&uri->query, "socket=%s", src->hosts->u.uds.path) < 0))
> goto cleanup;
>
> if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0)
> goto cleanup;
>
> - if (VIR_STRDUP(uri->server, src->hosts->name) < 0)
> - goto cleanup;
> + if (src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX)
> + if (VIR_STRDUP(uri->server, src->hosts->u.inet.addr) < 0)
> + goto cleanup;
This also should be converted to a switch.
>
> ret = virURIFormat(uri);
>
[...]
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 1011bd0..900a801 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -1603,9 +1603,12 @@ virStorageNetHostDefClear(virStorageNetHostDefPtr def)
> if (!def)
> return;
>
> - VIR_FREE(def->name);
> - VIR_FREE(def->port);
> - VIR_FREE(def->socket);
> + if (def->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) {
This is not extensible. Use switch and typecast if def->type is not of
the correct type.
> + VIR_FREE(def->u.uds.path);
> + } else {
> + VIR_FREE(def->u.inet.addr);
> + VIR_FREE(def->u.inet.port);
> + }
> }
>
>
> @@ -1643,6 +1646,9 @@ virStorageNetHostDefCopy(size_t nhosts,
> virStorageNetHostDefPtr ret = NULL;
> size_t i;
>
> + if (!hosts)
> + return NULL;
> +
This change is not relevant to the rest of the patch.
> if (VIR_ALLOC_N(ret, nhosts) < 0)
> goto error;
>
> @@ -1669,7 +1676,6 @@ virStorageNetHostDefCopy(size_t nhosts,
> return NULL;
> }
>
> -
Spurious whitespace change.
> void
> virStorageAuthDefFree(virStorageAuthDefPtr authdef)
> {
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
