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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list