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

Re: [libvirt] [PATCH 09/12] util: storagefile: Split out parsing of NBD string into a separate func



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

Attachment: signature.asc
Description: OpenPGP digital signature


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