Re: [Qemu-block] [PATCH v4 13/25] block/nbd: Implement bdrv_dirname()

[Max Reitz]

On 17.01.2017 00:21, Eric Blake wrote:
> On 01/16/2017 02:49 PM, Max Reitz wrote:
>> The idea behind this implementation is that the export name might be
>> interpreted as a path (which is the only apparent interpretation of
>> relative filenames for NBD paths).
>>
>> The default implementation of bdrv_dirname() would handle that just fine
>> for nbd+tcp, but not for nbd+unix, because in that case, the last
>> element of the path is the Unix socket path and not the export name.
>> Therefore, we need to implement an own bdrv_dirname() which uses the
>> legacy NBD URL which has the export name at the end.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  block/nbd.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 35f24be069..42f0cd638c 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -552,6 +552,34 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
>> QDict *options)
>>      bs->full_open_options = opts;
>>  }
>>  
>> +static char *nbd_dirname(BlockDriverState *bs, Error **errp)
>> +{
>> +    BDRVNBDState *s = bs->opaque;
>> +    const char *host = NULL, *port = NULL, *path = NULL;
>> +
>> +    if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) {
>> +        const InetSocketAddress *inet = s->saddr->u.inet.data;
>> +        if (!inet->has_ipv4 && !inet->has_ipv6 && !inet->has_to) {
>> +            host = inet->host;
>> +            port = inet->port;
>> +        }
>> +    } else if (s->saddr->type == SOCKET_ADDRESS_KIND_UNIX) {
>> +        path = s->saddr->u.q_unix.data->path;
>> +    }
>> +
> 
> Should we be catering to SOCKET_ADDRESS_KIND_VSOCK or
> SOCKET_ADDRESS_KIND_FD (if only to assert that they aren't possible with
> NBD)?

In those cases, the generic "Cannot generate..." error below will be
returned. I think that should be enough.

>> +    if (path) {
>> +        return g_strdup_printf("nbd:unix:%s:exportname=%s%s", path,
>> +                               s->export ?: "", s->export ? "/" : "");
>> +    } else if (host) {
>> +        return g_strdup_printf("nbd://%s:%s/%s%s", host, port,
>> +                               s->export ?: "", s->export ? "/" : "");
>> +    }
>> +
>> +    error_setg(errp, "Cannot generate a base directory for NBD node '%s'",
>> +               bs->filename);
>> +    return NULL;
>> +}
>> +
> 
> The NBD protocol doesn't require the server to support directories, but
> also doesn't place any requirements on export names, so this approach
> assumes a particular server behavior, but is probably as good as any
> other approach.  But I'm still not sold that we need this, vs. just
> declaring that NBD has no directory structure and therefore we always
> return NULL (even with nbd+tcp, and whether or not the older nbd: URI
> was used).  So I'm not quite ready for R-b on this one yet.

Right. Perhaps it's better to just always make it an error for now and
maybe revisit it later if someone asks for it (which I can't imagine...).

Max

signature.asc
Description: OpenPGP digital signature