4:56 a.m.
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