On 11/20/14 16:16, John Ferlan wrote:
On 11/12/2014 08:47 AM, Peter Krempa wrote:
Split out the code so that the function looks homogenous after adding more protocol specific parsers. --- src/util/virstoragefile.c | 141 ++++++++++++++++++++++++++++------------------ 1 file changed, 86 insertions(+), 55 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f267d1a..58be237 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2353,77 +2353,109 @@ virStorageSourceParseRBDColonString(const char *rbdstr,
static int -virStorageSourceParseBackingColon(virStorageSourcePtr src, - const char *path) +virStorageSourceParseNBDColonString(const char *nbdstr, + virStorageSourcePtr src) { char **backing = NULL; int ret = -1;
- if (!(backing = virStringSplit(path, ":", 0))) + if (!(backing = virStringSplit(nbdstr, ":", 0))) goto cleanup;
- if (!backing[0] || - (src->protocol = virStorageNetProtocolTypeFromString(backing[0])) < 0) { + /* we know that backing[0] now equals to "nbd" */ + + if (VIR_ALLOC_N(src->hosts, 1) < 0) + goto cleanup; + + src->nhosts = 1; + src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP; + + /* format: [] denotes optional sections, uppercase are variable strings + * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] + * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME] + */ + if (!backing[1]) {
IOW: If someone provided just "nbd:", right?
virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid backing protocol '%s'"), - NULLSTR(backing[0])); + _("missing remote information in '%s' for protocol nbd"), + nbdstr); goto cleanup; - } + } else if (STREQ(backing[1], "unix")) { + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing unix socket path in nbd backing string %s"), + nbdstr); + goto cleanup; + }
- switch ((virStorageNetProtocol) src->protocol) { - case VIR_STORAGE_NET_PROTOCOL_NBD: - if (VIR_ALLOC_N(src->hosts, 1) < 0) + if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) goto cleanup; - src->nhosts = 1; - src->hosts->transport = VIR_STORAGE_NET_HOST_TRANS_TCP;
- /* format: [] denotes optional sections, uppercase are variable strings - * nbd:unix:/PATH/TO/SOCKET[:exportname=EXPORTNAME] - * nbd:HOSTNAME:PORT[:exportname=EXPORTNAME] - */ + } else { if (!backing[1]) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing remote information in '%s' for protocol nbd"), - path); + _("missing host name in nbd string '%s'"), + nbdstr);
How could this ever have happened?
We have : if (!backing[1]) error else if (STREQ(backing[1], "unix")) ... else if (!backing[1]) different error
Yep, that doesn't make sense. That is probably an artifact from the time I wrote the func :/. I'll remove it in a follow up to keep the split-out patch intact in the semantic way.
goto cleanup; - } else if (STREQ(backing[1], "unix")) { - if (!backing[2]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing unix socket path in nbd backing string %s"), - path); - goto cleanup; - } + }
- if (VIR_STRDUP(src->hosts->socket, backing[2]) < 0) - goto cleanup; + if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) + goto cleanup;
- } else { - if (!backing[1]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing host name in nbd string '%s'"), - path); - goto cleanup; - } + if (!backing[2]) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("missing port in nbd string '%s'"), + nbdstr); + goto cleanup; + }
- if (VIR_STRDUP(src->hosts->name, backing[1]) < 0) - goto cleanup; + if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) + goto cleanup; + }
- if (!backing[2]) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("missing port in nbd string '%s'"), - path); - goto cleanup; - } + if (backing[3] && STRPREFIX(backing[3], "exportname=")) { + if (VIR_STRDUP(src->path, backing[3] + strlen("exportname=")) < 0) + goto cleanup; + }
If someone provides "nbd:HOSTNAME:PORT:exportnam=EXPORTNAME" by mistake are they never told of their error? I know - we're not in the business of validating mistakes, but it seems a shame to avoid the opportunity...
Yes we could do a better job here. Additionally there is a second function doing similar stuff in the qemu_command file. I'll post a patch to unify and clean up them as a follow up as this would be out of scope of the split-out patch.
ACK anyway,
John
- if (VIR_STRDUP(src->hosts->port, backing[2]) < 0) - goto cleanup; - } + ret = 0;
- if (backing[3] && STRPREFIX(backing[3], "exportname=")) { - if (VIR_STRDUP(src->path, backing[3] + strlen("exportname=")) < 0) - goto cleanup; - } - break; + cleanup: + virStringFreeList(backing); + + return ret; +} + + +static int +virStorageSourceParseBackingColon(virStorageSourcePtr src, + const char *path) +{ + char *protocol = NULL; + const char *p; + int ret = -1; + + if (!(p = strchr(path, ':'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol string '%s'"), + path); + goto cleanup; + } + + if (VIR_STRNDUP(protocol, path, p - path) < 0) + goto cleanup; + + if ((src->protocol = virStorageNetProtocolTypeFromString(protocol)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid backing protocol '%s'"), + protocol); + goto cleanup; + } + + switch ((virStorageNetProtocol) src->protocol) { + case VIR_STORAGE_NET_PROTOCOL_NBD: + if (virStorageSourceParseNBDColonString(path, src) < 0) + goto cleanup; + break;
case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: case VIR_STORAGE_NET_PROTOCOL_RBD: @@ -2431,7 +2463,7 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_NONE: virReportError(VIR_ERR_INTERNAL_ERROR, _("backing store parser is not implemented for protocol %s"), - backing[0]); + protocol); goto cleanup;
case VIR_STORAGE_NET_PROTOCOL_HTTP: @@ -2443,16 +2475,15 @@ virStorageSourceParseBackingColon(virStorageSourcePtr src, case VIR_STORAGE_NET_PROTOCOL_GLUSTER: virReportError(VIR_ERR_INTERNAL_ERROR, _("malformed backing store path for protocol %s"), - backing[0]); + protocol); goto cleanup; }
ret = 0;
cleanup: - virStringFreeList(backing); + VIR_FREE(protocol); return ret; - }
Peter