[libvirt] [PATCH 05/13] qemu: support NBD with Unix sockets

[Osier Yang]

On 2013年03月11日 21:20, Paolo Bonzini wrote:
> Il 11/03/2013 10:34, Osier Yang ha scritto:
>>>
>>> +    if (STRPREFIX(host, "unix:/")) {
>>> +        src = strchr(host + strlen("unix:"), ':');
>>> +        if (src)
>>> +            *src++ = '\0';
>>
>> Same comments as previous patches.
>
> I think in this case the value that is assigned to src is complex enough
> that it's better to keep it separate.

Okay, but still applies to other similar codes.

>
>>>
>>> -    *port++ = '\0';
>>> -    h->name = strdup(host);
>>> -    if (!h->name)
>>> -        goto no_memory;
>>> +        h->transport = VIR_DOMAIN_DISK_PROTO_TRANS_UNIX;
>>> +        h->socket = strdup(host + strlen("unix:"));
>>> +    } else {
>>> +        port = strchr(host, ':');
>>> +        if (!port) {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("cannot parse nbd filename '%s'"),
>>> disk->src);
>>> +            goto error;
>>> +        }
>>>
>>> -    src = strchr(port, ':');
>>> -    if (src)
>>> -        *src++ = '\0';
>>> +        *port++ = '\0';
>>> +        h->name = strdup(host);
>>> +        if (!h->name)
>>> +            goto no_memory;
>>>
>>> -    h->port = strdup(port);
>>> -    if (!h->port)
>>> -        goto no_memory;
>>> +        src = strchr(port, ':');
>>> +        if (src)
>>> +            *src++ = '\0';
>>> +
>>> +        h->port = strdup(port);
>>> +        if (!h->port)
>>> +            goto no_memory;
>>> +    }
>>>
>>>        if (src&&   STRPREFIX(src, "exportname=")) {
>>>            src = strdup(strchr(src, '=') + 1);
>>> @@ -2261,6 +2270,14 @@ qemuBuildNBDString(virDomainDiskDefPtr disk,
>>> virBufferPtr opt)
>>>            virBufferEscape(opt, ',', ",", ":%s",
>>>                            disk->hosts->port ? disk->hosts->port :
>>> "10809");
>>>            break;
>>> +    case VIR_DOMAIN_DISK_PROTO_TRANS_UNIX:
>>> +        if (!disk->hosts->socket) {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>>> +                           _("socket attribute required for unix
>>> transport"));
>>> +            return -1;
>>> +        }
>>
>> Not sure if we should do this when parsing, as I think the "socket" is
>> required as long as the transport protocol is "unix", unless it's
>> generated somewhere.
>
> Yeah, we cannot be sure in general that it will be required for _all_
> protocols, so I kept it here and followed what gluster does.
>

Agreed.