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

Re: [libvirt] [PATCH] virStorageNetHostDef: Turn @port into integer



On 07/19/2017 05:58 PM, Peter Krempa wrote:
> On Wed, Jul 19, 2017 at 17:26:27 +0200, Michal Privoznik wrote:
>> Currently, @port is type of string. Well, that's overkill and
>> waste of memory. Port is always an integer. Use it as such.
>>
>> Signed-off-by: Michal Privoznik <mprivozn redhat com>
>> ---
>>  src/conf/domain_conf.c                |  25 ++++++--
>>  src/libxl/libxl_conf.c                |   2 +-
>>  src/qemu/qemu_block.c                 |   2 +-
>>  src/qemu/qemu_command.c               |  28 ++-------
>>  src/qemu/qemu_parse_command.c         |  33 +++++++---
>>  src/storage/storage_backend_gluster.c |  17 ++---
>>  src/storage/storage_driver.c          |   7 +--
>>  src/util/virstoragefile.c             | 113 +++++++++++++++++++++-------------
>>  src/util/virstoragefile.h             |   4 +-
>>  src/xenconfig/xen_xl.c                |   2 +-
>>  10 files changed, 130 insertions(+), 103 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 9320794de..e8fdaa36f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
> 
> [...]
> 
>> @@ -6486,7 +6487,15 @@ virDomainStorageHostParse(xmlNodePtr node,
>>                      goto cleanup;
>>                  }
>>  
>> -                host.port = virXMLPropString(child, "port");
>> +                port =  virXMLPropString(child, "port");
>> +                if (port &&
>> +                    (virStrToLong_i(port, NULL, 10, &host.port) < 0 ||
>> +                     host.port < 0)) {
>> +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                                   _("failed to parse port number '%s'"),
>> +                                   port);
>> +                    goto cleanup;
>> +                }
>>              }
> 
> 'port' is leaked when multiple <host> element are passed.
> 
>>  
>>              if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0)
> 
> 
> 
>> @@ -14909,7 +14919,7 @@ virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first,
>>          &second->source.subsys.u.scsi.u.iscsi;
>>  
>>      if (STREQ(first_iscsisrc->hosts[0].name, second_iscsisrc->hosts[0].name) &&
>> -        STREQ(first_iscsisrc->hosts[0].port, second_iscsisrc->hosts[0].port) &&
>> +        first_iscsisrc->hosts[0].port == second_iscsisrc->hosts[0].port &&
>>          STREQ(first_iscsisrc->path, second_iscsisrc->path))
>>          return 1;
>>      return 0;
> 
> 
>> @@ -22255,7 +22267,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf,
>>          if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
>>              virBufferAddLit(buf, "<host");
>>              virBufferEscapeString(buf, " name='%s'", iscsisrc->hosts[0].name);
>> -            virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].port);
>> +            if (iscsisrc->hosts[0].port)
>> +                virBufferAsprintf(buf, " port='%d'", iscsisrc->hosts[0].port);
>>              virBufferAddLit(buf, "/>\n");
>>          } else {
>>              virBufferAsprintf(buf, "<adapter name='%s'/>\n",
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index a85bc71d2..1b20fb906 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -706,7 +706,7 @@ libxlMakeNetworkDiskSrcStr(virStorageSourcePtr src,
>>                      virBufferAsprintf(&buf, "%s", src->hosts[i].name);
>>  
>>                  if (src->hosts[i].port)
>> -                    virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port);
>> +                    virBufferAsprintf(&buf, "\\:%d", src->hosts[i].port);
>>              }
>>          }
>>  
>> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>> index 93124c5ba..3d64528a8 100644
>> --- a/src/qemu/qemu_block.c
>> +++ b/src/qemu/qemu_block.c
>> @@ -465,7 +465,7 @@ qemuBlockStorageSourceBuildHostsJSONSocketAddress(virStorageSourcePtr src,
>>              if (virJSONValueObjectCreate(&server,
>>                                           "s:type", transport,
>>                                           "s:host", host->name,
>> -                                         "s:port", host->port,
>> +                                         "i:port", host->port,
>>                                           NULL) < 0)
>>                  goto cleanup;
>>              break;
> 
> 
> 
>> @@ -897,8 +880,7 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>>  
>>                  switch (src->hosts->transport) {
>>                  case VIR_STORAGE_NET_HOST_TRANS_TCP:
>> -                    virBufferStrcat(&buf, src->hosts->name, ":",
>> -                                    src->hosts->port, NULL);
>> +                    virBufferAsprintf(&buf, "%s:%d", src->hosts->name, src->hosts->port);
> 
> Line too long
> 
>> @@ -953,9 +935,9 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src,
>>                  if (virAsprintf(&ret, "sheepdog:%s", src->path) < 0)
>>                      goto cleanup;
>>              } else if (src->nhosts == 1) {
>> -                if (virAsprintf(&ret, "sheepdog:%s:%s:%s",
>> +                if (virAsprintf(&ret, "sheepdog:%s:%d:%s",
>>                                  src->hosts->name,
>> -                                src->hosts->port ? src->hosts->port : "7000",
>> +                                src->hosts->port ? src->hosts->port : 7000,
> 
> Hmm, this was missed in my cleanup patch.
> 
>>                                  src->path) < 0)
>>                      goto cleanup;
>>              } else {
> 
> [...]
> 
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 2d0ff7812..bea4a42ad 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1688,8 +1688,8 @@ virStorageNetHostDefClear(virStorageNetHostDefPtr def)
>>      if (!def)
>>          return;
>>  
>> +    def->port = 0;
> 
> 'transport' is not reset here, so port doesn't need to be either.
> 
>>      VIR_FREE(def->name);
>> -    VIR_FREE(def->port);
>>      VIR_FREE(def->socket);
> 
> 
> 
>> @@ -2430,10 +2428,8 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src,
>>          tmp[0] = '\0';
>>      }
>>  
>> -    if (uri->port > 0) {
>> -        if (virAsprintf(&src->hosts->port, "%d", uri->port) < 0)
>> -            goto cleanup;
>> -    }
>> +    if (uri->port > 0)
>> +        src->hosts->port = uri->port;
> 
> The condition is redundant.
> 
>>  
>>      if (VIR_STRDUP(src->hosts->name, uri->server) < 0)
>>          goto cleanup;
> 
> [...]
> 
>> @@ -2638,7 +2639,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr,
>>          if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0)
>>              goto cleanup;
>>  
>> -   } else {
>> +    } else {
> 
> Surious change.
> 
>>          if (VIR_STRDUP(src->hosts->name, backing[1]) < 0)
>>              goto cleanup;
>>  
> 
> [...]
> 
>> @@ -2985,11 +2999,12 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src,
>>          goto cleanup;
>>  
>>      if ((port = strchr(src->hosts->name, ':'))) {
>> -        if (VIR_STRDUP(src->hosts->port, port + 1) < 0)
>> -            goto cleanup;
>> -
>> -        if (strlen(src->hosts->port) == 0)
>> -            VIR_FREE(src->hosts->port);
>> +        if (virStrToLong_i(port + 1, NULL, 10, &src->hosts->port) < 0 ||
> 
> This will report the error if the port is not specified after the colon,
> whereas previously it would not.
> 
>> +            src->hosts->port < 0) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("failed to parse port number '%s'"),
>> +                           port + 1);
>> +        }
>>  
>>          *port = '\0';
>>      }
> 
> 
> [...]
> 
>> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
>> index 98992e04a..934504806 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -155,7 +155,7 @@ typedef struct _virStorageNetHostDef virStorageNetHostDef;
>>  typedef virStorageNetHostDef *virStorageNetHostDefPtr;
>>  struct _virStorageNetHostDef {
>>      char *name;
>> -    char *port;
>> +    int port;
> 
> If you want to be precise ... have you ever seen negative ports?
> 
>>      int transport; /* virStorageNetHostTransport */
>>      char *socket;  /* path to unix socket */
>>  };
> 
> This will require a lot of fixing since you blindly copied the check
> that also checks that the port is not less than 0.
> 

Okay, I don't care that much to post v2. If somebody wants to take it
over from here, be my guest.

Michal


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