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; }; This patch performs the required changes in transforming _virStorageNetHostDef to fit union discrimination. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com&gt; --- 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)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown protocol transport type '%s'"), transport); @@ -5989,17 +5991,17 @@ virDomainStorageHostParse(xmlNodePtr node, } } - host.socket = virXMLPropString(child, "socket"); + socket = virXMLPropString(child, "socket"); - if (host.transport == VIR_STORAGE_NET_HOST_TRANS_UNIX && - host.socket == NULL) { + if (host.type == VIR_STORAGE_NET_HOST_TRANS_UNIX && + socket == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing socket for unix transport")); goto cleanup; } - if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX && - host.socket != NULL) { + if (host.type != VIR_STORAGE_NET_HOST_TRANS_UNIX && + socket != NULL) { virReportError(VIR_ERR_XML_ERROR, _("transport '%s' does not support " "socket attribute"), @@ -6007,16 +6009,17 @@ virDomainStorageHostParse(xmlNodePtr node, goto cleanup; } + host.u.uds.path = socket; VIR_FREE(transport); - if (host.transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { - if (!(host.name = virXMLPropString(child, "name"))) { + if (host.type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (!(host.u.inet.addr = virXMLPropString(child, "name"))) { virReportError(VIR_ERR_XML_ERROR, "%s", _("missing name for host")); goto cleanup; } - host.port = virXMLPropString(child, "port"); + host.u.inet.port = virXMLPropString(child, "port"); } if (VIR_APPEND_ELEMENT(*hosts, *nhosts, host) < 0) @@ -14065,8 +14068,8 @@ virDomainHostdevMatchSubsysSCSIiSCSI(virDomainHostdevDefPtr first, virDomainHostdevSubsysSCSIiSCSIPtr second_iscsisrc = &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) && + if (STREQ(first_iscsisrc->hosts[0].u.inet.addr, second_iscsisrc->hosts[0].u.inet.addr) && + STREQ(first_iscsisrc->hosts[0].u.inet.port, second_iscsisrc->hosts[0].u.inet.port) && STREQ(first_iscsisrc->path, second_iscsisrc->path)) return 1; return 0; @@ -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"); } @@ -21103,8 +21109,8 @@ virDomainHostdevDefFormatSubsys(virBufferPtr buf, case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI: 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); + virBufferEscapeString(buf, " name='%s'", iscsisrc->hosts[0].u.inet.addr); + virBufferEscapeString(buf, " port='%s'", iscsisrc->hosts[0].u.inet.port); virBufferAddLit(buf, "/>\n"); } else { virBufferAsprintf(buf, "<adapter name='%s'/>\n", 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; if (virJSONValueObjectAdd(server, - "s:host", host->name, + "s:host", host->u.inet.addr, "s:port", portstr, NULL) < 0) goto cleanup; @@ -842,7 +841,7 @@ qemuBuildGlusterDriveJSONHosts(virStorageSourcePtr src) case VIR_STORAGE_NET_HOST_TRANS_UNIX: if (virJSONValueObjectAdd(server, - "s:socket", host->socket, + "s:socket", host->u.uds.path, NULL) < 0) goto cleanup; break; @@ -916,19 +915,19 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, if (VIR_ALLOC(uri) < 0) goto cleanup; - if (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { + if (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_TCP) { if (VIR_STRDUP(uri->scheme, virStorageNetProtocolTypeToString(src->protocol)) < 0) goto cleanup; } else { if (virAsprintf(&uri->scheme, "%s+%s", virStorageNetProtocolTypeToString(src->protocol), - virStorageNetHostTransportTypeToString(src->hosts->transport)) < 0) + virStorageNetHostTransportTypeToString(src->hosts->type)) < 0) goto cleanup; } if ((uri->port = qemuNetworkDriveGetPort(src->protocol, - src->hosts->port)) < 0) + src->hosts->u.inet.port)) < 0) goto cleanup; if (src->path) { @@ -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; ret = virURIFormat(uri); @@ -979,38 +979,38 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, goto cleanup; } - if (!((src->hosts->name && strchr(src->hosts->name, ':')) || - (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_TCP && - !src->hosts->name) || - (src->hosts->transport == VIR_STORAGE_NET_HOST_TRANS_UNIX && - src->hosts->socket && - src->hosts->socket[0] != '/'))) { + if (!((src->hosts->u.inet.addr && strchr(src->hosts->u.inet.addr, ':')) || + (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_TCP && + !src->hosts->u.inet.addr) || + (src->hosts->type == VIR_STORAGE_NET_HOST_TRANS_UNIX && + src->hosts->u.uds.path && + src->hosts->u.uds.path[0] != '/'))) { virBufferAddLit(&buf, "nbd:"); - switch (src->hosts->transport) { + switch (src->hosts->type) { case VIR_STORAGE_NET_HOST_TRANS_TCP: - virBufferStrcat(&buf, src->hosts->name, NULL); + virBufferStrcat(&buf, src->hosts->u.inet.addr, NULL); virBufferAsprintf(&buf, ":%s", - src->hosts->port ? src->hosts->port : + src->hosts->u.inet.port ? src->hosts->u.inet.port : QEMU_DEFAULT_NBD_PORT); break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: - if (!src->hosts->socket) { + if (!src->hosts->u.uds.path) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("socket attribute required for " "unix transport")); goto cleanup; } - virBufferAsprintf(&buf, "unix:%s", src->hosts->socket); + virBufferAsprintf(&buf, "unix:%s", src->hosts->u.uds.path); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, _("nbd does not support transport '%s'"), - virStorageNetHostTransportTypeToString(src->hosts->transport)); + virStorageNetHostTransportTypeToString(src->hosts->type)); goto cleanup; } @@ -1049,8 +1049,8 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, goto cleanup; } else if (src->nhosts == 1) { if (virAsprintf(&ret, "sheepdog:%s:%s:%s", - src->hosts->name, - src->hosts->port ? src->hosts->port : "7000", + src->hosts->u.inet.addr, + src->hosts->u.inet.port ? src->hosts->u.inet.port : "7000", src->path) < 0) goto cleanup; } else { @@ -1084,14 +1084,14 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, virBufferAddLit(&buf, "\\;"); /* assume host containing : is ipv6 */ - if (strchr(src->hosts[i].name, ':')) + if (strchr(src->hosts[i].u.inet.addr, ':')) virBufferEscape(&buf, '\\', ":", "[%s]", - src->hosts[i].name); + src->hosts[i].u.inet.addr); else - virBufferAsprintf(&buf, "%s", src->hosts[i].name); + virBufferAsprintf(&buf, "%s", src->hosts[i].u.inet.addr); - if (src->hosts[i].port) - virBufferAsprintf(&buf, "\\:%s", src->hosts[i].port); + if (src->hosts[i].u.inet.port) + virBufferAsprintf(&buf, "\\:%s", src->hosts[i].u.inet.port); } } diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 405e655..41a8172 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -77,10 +77,10 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } if (!transp) { - def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + def->src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; } else { - def->src->hosts->transport = virStorageNetHostTransportTypeFromString(transp); - if (def->src->hosts->transport < 0) { + def->src->hosts->type = virStorageNetHostTransportTypeFromString(transp); + if (def->src->hosts->type < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid %s transport type '%s'"), scheme, transp); goto error; @@ -88,19 +88,19 @@ qemuParseDriveURIString(virDomainDiskDefPtr def, virURIPtr uri, } def->src->nhosts = 0; /* set to 1 once everything succeeds */ - if (def->src->hosts->transport != VIR_STORAGE_NET_HOST_TRANS_UNIX) { - if (VIR_STRDUP(def->src->hosts->name, uri->server) < 0) + if (def->src->hosts->type != VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (VIR_STRDUP(def->src->hosts->u.inet.addr, uri->server) < 0) goto error; - if (virAsprintf(&def->src->hosts->port, "%d", uri->port) < 0) + if (virAsprintf(&def->src->hosts->u.inet.port, "%d", uri->port) < 0) goto error; } else { - def->src->hosts->name = NULL; - def->src->hosts->port = 0; + def->src->hosts->u.inet.addr = NULL; + def->src->hosts->u.inet.port = 0; if (uri->query) { if (STRPREFIX(uri->query, "socket=")) { sock = strchr(uri->query, '=') + 1; - if (VIR_STRDUP(def->src->hosts->socket, sock) < 0) + if (VIR_STRDUP(def->src->hosts->u.uds.path, sock) < 0) goto error; } else { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -221,8 +221,8 @@ qemuParseNBDString(virDomainDiskDefPtr disk) if (src) *src++ = '\0'; - h->transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; - if (VIR_STRDUP(h->socket, host + strlen("unix:")) < 0) + h->type = VIR_STORAGE_NET_HOST_TRANS_UNIX; + if (VIR_STRDUP(h->u.uds.path, host + strlen("unix:")) < 0) goto error; } else { port = strchr(host, ':'); @@ -233,14 +233,14 @@ qemuParseNBDString(virDomainDiskDefPtr disk) } *port++ = '\0'; - if (VIR_STRDUP(h->name, host) < 0) + if (VIR_STRDUP(h->u.inet.addr, host) < 0) goto error; src = strchr(port, ':'); if (src) *src++ = '\0'; - if (VIR_STRDUP(h->port, port) < 0) + if (VIR_STRDUP(h->u.inet.port, port) < 0) goto error; } @@ -729,11 +729,10 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_ALLOC(def->src->hosts) < 0) goto error; def->src->nhosts = 1; - def->src->hosts->name = def->src->path; - if (VIR_STRDUP(def->src->hosts->port, port) < 0) + def->src->hosts->u.inet.addr = def->src->path; + if (VIR_STRDUP(def->src->hosts->u.inet.port, port) < 0) goto error; - def->src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src->hosts->socket = NULL; + def->src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; if (VIR_STRDUP(def->src->path, vdi) < 0) goto error; } @@ -2006,8 +2005,8 @@ qemuParseCommandLine(virCapsPtr caps, if (VIR_ALLOC(disk->src->hosts) < 0) goto error; disk->src->nhosts = 1; - disk->src->hosts->name = disk->src->path; - if (VIR_STRDUP(disk->src->hosts->port, port) < 0) + disk->src->hosts->u.inet.addr = disk->src->path; + if (VIR_STRDUP(disk->src->hosts->u.inet.port, port) < 0) goto error; if (VIR_STRDUP(disk->src->path, vdi) < 0) goto error; @@ -2548,14 +2547,13 @@ qemuParseCommandLine(virCapsPtr caps, goto error; } } - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].port = port; - if (VIR_STRDUP(first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].name, + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].u.inet.port = port; + if (VIR_STRDUP(first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].u.inet.addr, token) < 0) { VIR_FREE(hosts); goto error; } - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].socket = NULL; + first_rbd_disk->src->hosts[first_rbd_disk->src->nhosts].type = VIR_STORAGE_NET_HOST_TRANS_TCP; first_rbd_disk->src->nhosts++; token = strtok_r(NULL, ",", &saveptr); diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 8e86704..0d5b7f6 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -553,7 +553,8 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; VIR_DEBUG("deinitializing gluster storage file %p (gluster://%s:%s/%s%s)", - src, src->hosts->name, src->hosts->port ? src->hosts->port : "0", + src, src->hosts->u.inet.addr, + src->hosts->u.inet.port ? src->hosts->u.inet.port : "0", src->volume, src->path); if (priv->vol) @@ -568,27 +569,27 @@ static int virStorageFileBackendGlusterInitServer(virStorageFileBackendGlusterPrivPtr priv, virStorageNetHostDefPtr host) { - const char *transport = virStorageNetHostTransportTypeToString(host->transport); + const char *transport = virStorageNetHostTransportTypeToString(host->type); const char *hoststr = NULL; int port = 0; - switch ((virStorageNetHostTransport) host->transport) { + switch ((virStorageNetHostTransport) host->type) { case VIR_STORAGE_NET_HOST_TRANS_RDMA: case VIR_STORAGE_NET_HOST_TRANS_TCP: - hoststr = host->name; + hoststr = host->u.inet.addr; - if (host->port && - virStrToLong_i(host->port, NULL, 10, &port) < 0) { + if (host->u.inet.port && + virStrToLong_i(host->u.inet.port, NULL, 10, &port) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse port number '%s'"), - host->port); + host->u.inet.port); return -1; } break; case VIR_STORAGE_NET_HOST_TRANS_UNIX: - hoststr = host->socket; + hoststr = host->u.uds.path; break; case VIR_STORAGE_NET_HOST_TRANS_LAST: @@ -797,8 +798,8 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) return NULL; ignore_value(virAsprintf(&priv->canonpath, "gluster://%s:%s/%s/%s", - src->hosts->name, - src->hosts->port, + src->hosts->u.inet.addr, + src->hosts->u.inet.port, src->volume, filePath)); diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index a79acc6..7c7fddd 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -3355,10 +3355,10 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, if (VIR_ALLOC_N(def->src->hosts, def->src->nhosts) < 0) goto cleanup; - if (VIR_STRDUP(def->src->hosts[0].name, pooldef->source.hosts[0].name) < 0) + if (VIR_STRDUP(def->src->hosts[0].u.inet.addr, pooldef->source.hosts[0].name) < 0) goto cleanup; - if (virAsprintf(&def->src->hosts[0].port, "%d", + if (virAsprintf(&def->src->hosts[0].u.inet.port, "%d", pooldef->source.hosts[0].port ? pooldef->source.hosts[0].port : 3260) < 0) @@ -3384,8 +3384,7 @@ virStorageAddISCSIPoolSourceHost(virDomainDiskDefPtr def, /* Storage pool have not supported these 2 attributes yet, * use the defaults. */ - def->src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - def->src->hosts[0].socket = NULL; + def->src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; def->src->protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; 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) { + 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; + if (VIR_ALLOC_N(ret, nhosts) < 0) goto error; @@ -1650,16 +1656,17 @@ virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr src = &hosts[i]; virStorageNetHostDefPtr dst = &ret[i]; - dst->transport = src->transport; - - if (VIR_STRDUP(dst->name, src->name) < 0) - goto error; + dst->type = src->type; - if (VIR_STRDUP(dst->port, src->port) < 0) - goto error; - - if (VIR_STRDUP(dst->socket, src->socket) < 0) - goto error; + if (src->type == VIR_STORAGE_NET_HOST_TRANS_UNIX) { + if (VIR_STRDUP(dst->u.uds.path, src->u.uds.path) < 0) + goto error; + } else { + if (VIR_STRDUP(dst->u.inet.addr, src->u.inet.addr)< 0) + goto error; + if (VIR_STRDUP(dst->u.inet.port, src->u.inet.port)< 0) + goto error; + } } return ret; @@ -1669,7 +1676,6 @@ virStorageNetHostDefCopy(size_t nhosts, return NULL; } - void virStorageAuthDefFree(virStorageAuthDefPtr authdef) { @@ -2292,7 +2298,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, } if (scheme[1] && - (src->hosts->transport = virStorageNetHostTransportTypeFromString(scheme[1])) < 0) { + (src->hosts->type = virStorageNetHostTransportTypeFromString(scheme[1])) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid protocol transport type '%s'"), scheme[1]); @@ -2301,7 +2307,7 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, /* handle socket stored as a query */ if (uri->query) { - if (VIR_STRDUP(src->hosts->socket, STRSKIP(uri->query, "socket=")) < 0) + if (VIR_STRDUP(src->hosts->u.uds.path, STRSKIP(uri->query, "socket=")) < 0) goto cleanup; } @@ -2339,11 +2345,11 @@ virStorageSourceParseBackingURI(virStorageSourcePtr src, } if (uri->port > 0) { - if (virAsprintf(&src->hosts->port, "%d", uri->port) < 0) + if (virAsprintf(&src->hosts->u.inet.port, "%d", uri->port) < 0) goto cleanup; } - if (VIR_STRDUP(src->hosts->name, uri->server) < 0) + if (VIR_STRDUP(src->hosts->u.inet.addr, uri->server) < 0) goto cleanup; ret = 0; @@ -2378,26 +2384,25 @@ virStorageSourceRBDAddHost(virStorageSourcePtr src, if (port) { *port = '\0'; port += skip; - if (VIR_STRDUP(src->hosts[src->nhosts - 1].port, port) < 0) + if (VIR_STRDUP(src->hosts[src->nhosts - 1].u.inet.port, port) < 0) goto error; } parts = virStringSplit(hostport, "\\:", 0); if (!parts) goto error; - src->hosts[src->nhosts-1].name = virStringListJoin((const char **)parts, ":"); + src->hosts[src->nhosts-1].u.inet.addr = virStringListJoin((const char **)parts, ":"); virStringListFree(parts); - if (!src->hosts[src->nhosts-1].name) + if (!src->hosts[src->nhosts-1].u.inet.addr) goto error; - src->hosts[src->nhosts-1].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - src->hosts[src->nhosts-1].socket = NULL; + src->hosts[src->nhosts-1].type = VIR_STORAGE_NET_HOST_TRANS_TCP; return 0; error: - VIR_FREE(src->hosts[src->nhosts-1].port); - VIR_FREE(src->hosts[src->nhosts-1].name); + VIR_FREE(src->hosts[src->nhosts-1].u.inet.port); + VIR_FREE(src->hosts[src->nhosts-1].u.inet.addr); return -1; } @@ -2524,7 +2529,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; src->nhosts = 1; - src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + src->hosts->type = VIR_STORAGE_NET_HOST_TRANS_TCP; /* format: [] denotes optional sections, uppercase are variable strings * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] @@ -2543,7 +2548,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) + if (VIR_STRDUP(src->hosts->u.uds.path, backing[2]) < 0) goto cleanup; } else { @@ -2554,7 +2559,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) + if (VIR_STRDUP(src->hosts->u.inet.addr, backing[1]) < 0) goto cleanup; if (!backing[2]) { @@ -2564,7 +2569,7 @@ virStorageSourceParseNBDColonString(const char *nbdstr, goto cleanup; } - if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) + if (VIR_STRDUP(src->hosts->u.inet.port, backing[2]) < 0) goto cleanup; } @@ -2724,7 +2729,7 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, return -1; } - host->transport = transport; + host->type = transport; switch ((virStorageNetHostTransport) transport) { case VIR_STORAGE_NET_HOST_TRANS_TCP: @@ -2735,8 +2740,8 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, return -1; } - if (VIR_STRDUP(host->name, hostname) < 0 || - VIR_STRDUP(host->port, port) < 0) + if (VIR_STRDUP(host->u.inet.addr, hostname) < 0 || + VIR_STRDUP(host->u.inet.port, port) < 0) return -1; break; @@ -2749,7 +2754,7 @@ virStorageSourceParseBackingJSONGlusterHost(virStorageNetHostDefPtr host, } - if (VIR_STRDUP(host->socket, socket) < 0) + if (VIR_STRDUP(host->u.uds.path, socket) < 0) return -1; break; @@ -2866,15 +2871,15 @@ virStorageSourceParseBackingJSONNbd(virStorageSourcePtr src, src->nhosts = 1; if (path) { - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; - if (VIR_STRDUP(src->hosts[0].socket, path) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_UNIX; + if (VIR_STRDUP(src->hosts[0].u.uds.path, path) < 0) return -1; } else { - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - if (VIR_STRDUP(src->hosts[0].name, host) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; + if (VIR_STRDUP(src->hosts[0].u.inet.addr, host) < 0) return -1; - if (VIR_STRDUP(src->hosts[0].port, port) < 0) + if (VIR_STRDUP(src->hosts[0].u.inet.port, port) < 0) return -1; } @@ -2932,11 +2937,11 @@ virStorageSourceParseBackingJSONSSH(virStorageSourcePtr src, return -1; src->nhosts = 1; - src->hosts[0].transport = VIR_STORAGE_NET_HOST_TRANS_TCP; - if (VIR_STRDUP(src->hosts[0].name, host) < 0) + src->hosts[0].type = VIR_STORAGE_NET_HOST_TRANS_TCP; + if (VIR_STRDUP(src->hosts[0].u.inet.addr, host) < 0) return -1; - if (VIR_STRDUP(src->hosts[0].port, port) < 0) + if (VIR_STRDUP(src->hosts[0].u.inet.port, port) < 0) return -1; return 0; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3d09468..99b4a31 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -149,13 +149,25 @@ typedef enum { VIR_ENUM_DECL(virStorageNetHostTransport) +typedef struct _virStorageNetHostUnixSockAddr virStorageNetHostUnixSockAddr; +struct _virStorageNetHostUnixSockAddr { + char *path; +}; + +typedef struct _virStorageNetHostInetSockAddr virStorageNetHostInetSockAddr; +struct _virStorageNetHostInetSockAddr { + char *addr; + char *port; +}; + typedef struct _virStorageNetHostDef virStorageNetHostDef; typedef virStorageNetHostDef *virStorageNetHostDefPtr; struct _virStorageNetHostDef { - char *name; - char *port; - int transport; /* virStorageNetHostTransport */ - char *socket; /* path to unix socket */ + virStorageNetHostTransport type; + union { /* union tag is @type */ + virStorageNetHostUnixSockAddr uds; + virStorageNetHostInetSockAddr inet; + } u; }; /* Information for a storage volume from a virStoragePool */ diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f766df1..5c685dd 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -387,7 +387,7 @@ testStorageChain(const void *args) elt->type, elt->format, virStorageNetProtocolTypeToString(elt->protocol), - NULLSTR(elt->nhosts ? elt->hosts[0].name : NULL), + NULLSTR(elt->nhosts ? elt->hosts[0].u.inet.addr : NULL), NULLSTR(elt->auth ? elt->auth->username : NULL)) < 0) { VIR_FREE(expect); VIR_FREE(actual); -- 2.7.4

On Wed, Dec 7, 2016 at 7:31 PM, Peter Krempa <pkrempa(a)redhat.com&gt; wrote: 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

On Tue, Dec 06, 2016 at 22:51:59 +0530, Prasanna Kumar Kalever wrote: A bit clearer explanation would help. This is mostly relevat for VM startup where the multiple calls are done. VM shutdown currently calls just one operation, but is not optimized with this patch. Read below for a suggestion of a farbetter approach than having the 'initFlag' variable in all places that call the code. [...] I'd call this 'deinitsrc'. You should export and reuse virStorageFileIsInitialized rather than reimplementing the detection. The above code is confusing and does not conform to the coding style. Drop the 'else' statement completely, since the first part jumps to cleanup anyways. Also snap->src->drv won't ever be initialized. the 'snap' object is freshly created from the XML provided by the user so it never was part of the definition and thus couldn't ever be initialized. This condition is always true due to the fact above. Initializing the domain disks is useless at this point. The new snapshot volumes will be accessed, not the existing disks. This code also is not necessary for internal snapshots and thus should not be executed in such cases. This wrapper is rather useless. You can call virStorageFileDeinit on volume files that were not initialized. This does not conform to the coding style. Multi line bodies need to be included in a block. If it's requested not to relabel the file as seed just below the code you added, you don't need to initialize the volume at all. In the above case, this will need to be skipped as well. [...] Alternatively and preferably you could turn src->drv into a object. This would add the option to do reference counting and would get rid of the need to remember whether a given piece of code needs to deinitialize a storage volume. Most of the changes above would not be necessary then. Peter

On Mon, Dec 12, 2016 at 20:01:35 +0530, Prasanna Kalever wrote: [...] Yes I thought you were not initializing it originally. That's fine then. ... ... That's true. But if you make sure that the caller always initializes it, the condition will become always false. Yes you are right. I was probably looking ad a different function, since libvirt almost everywhere uses 'def' for the domain definition. Yes that's right. Also you should execute this code after the call to virDomainSnapshotAlignDisks and qemuDomainSnapshotPrepare. They are checking various stuff and determining the default location for the disks, so the initialization would fail in some cases otherwise. [...] Okay. It may make sense in the future to have all the exit points marked using a special function. [...] [...] I meant to make it a virObject, so that you can refcount it. The need for a flag to track whether it was initialized is rather fragile.

Let the metadata for storage driver access to remote and local volumes be cleaned by its respective driver *Deinit methods. This will be used in the next patch, which will implement a connection cache for/in gluster protocol driver. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever(a)redhat.com&gt; --- src/storage/storage_backend_fs.c | 2 ++ src/storage/storage_backend_gluster.c | 3 ++- src/storage/storage_driver.c | 5 +---- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index de0e8d5..0e03e06 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1488,6 +1488,8 @@ virStorageFileBackendFileDeinit(virStorageSourcePtr src) VIR_FREE(priv->canonpath); VIR_FREE(priv); + + VIR_FREE(src->drv); } diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0d5b7f6..e0841ca 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -562,7 +562,8 @@ virStorageFileBackendGlusterDeinit(virStorageSourcePtr src) VIR_FREE(priv->canonpath); VIR_FREE(priv); - src->drv->priv = NULL; + + VIR_FREE(src->drv); } static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 24e2f35..2a4e160 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2900,11 +2900,8 @@ virStorageFileDeinit(virStorageSourcePtr src) if (!virStorageFileIsInitialized(src)) return; - if (src->drv->backend && - src->drv->backend->backendDeinit) + if (src->drv->backend && src->drv->backend->backendDeinit) src->drv->backend->backendDeinit(src); - - VIR_FREE(src->drv); } -- 2.7.4

On Wed, Dec 7, 2016 at 7:39 PM, Peter Krempa <pkrempa(a)redhat.com&gt; wrote: Something like ? Apologies, I don't understand your intention here, with some clues and if it is needed I can make it better. Okay, will discard this patch Thanks, -- Prasanna

On Tue, Dec 06, 2016 at 22:52:01 +0530, Prasanna Kumar Kalever wrote: This part was extracted to a separate patch already, thus this doesn't have to deal with snapshots in any way than with other gluster connections. You should also keep a reference to the object in the cache, so that you can return it and don't have to look it up again. You don't really need the complete object here. you only need the glfs_t part here. The 'cannonpath' variable is different for possible storage source objects. As said. Don't use global flags. You should be able to avoid both of them. Okay. I've seen below. They not only are avoidable, but your usage is outright dangerous. I've noticed that you actually create a lockable object here, but don't use the locks. This is currently okay, since it's write only. The lock might come handy though ... Umm ... you iterate the cache ... ... before attempting to initialize the global cache?!? That doesn't make sense. Also this call is not needed here, since the only entrypoint is virStorageFileBackendGlusterInit which initializes it. You leak newConn in some cases. We usually use "Lookup". This name is weird. Did you mean "Release"? Umm, nope, that's not how it works. This is totaly thread unsafe. Other thread can modify your global flag. You probably did not check how virObjectUnref actually works. The return value notifes you if the object was disposed (you removed the last referernce) or not. This leaks src->drv in cases where the entry was used by two objects simultaneously. This needs to be done in the calling function. (As also pointed out in one of the other patches) So yet another thread-unsafe part. You correctly add the not-yet-opened connection to the cache, but there is no mechanism to guard suff after this. If a second thread obtains the cache entry wile the first one is not yet finished opening the connection it will get an invalid one. Any thread obtaining an entry from the cache needs to check whether the conection was initialized already as the first thing to do. It can continue only if it's already open and otherwise needs to wait until the original thread finishes. The first thread that added the object into the cache needs to open the connection and notify all other threads potentialy waiting on the condition that it was opened. A virCond should help.

On Mon, Dec 12, 2016 at 8:02 PM, Prasanna Kalever <pkalever(a)redhat.com&gt; wrote: Sorry, this still holds good. on taking a snapshot virStorageFileGetMetadataRecurse will be called on each overlay. Which seeks/hits the cache help while the virStorageFileInitAs respective calls (In case if snaps reside on the same volume). Thanks, -- Prasanna