On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa@xxxxxxxxxx> wrote:
> 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.
HUhhh...
How to make sure that I have covered all the drivers ?
Any long list of all drivers in libvirt or config to compile all the modules ?
Thanks,
--
Prasanna
>
> 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)
>> {
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
