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

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(a)redhat.com&gt; > --- > 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.