Re: [PATCH v4 1/4] util: change the virStorageNetHostDef type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux