Re: [Qemu-devel] [PATCH 16/19] block/nbd: Implement bdrv_dirname()

[Max Reitz]

On 02.05.2016 17:36, Kevin Wolf wrote:
> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben:
>> 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 | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index f7ea3b3..64534bb 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -469,6 +469,32 @@ static void nbd_refresh_filename(BlockDriverState *bs, 
>> QDict *options)
>>      bs->full_open_options = opts;
>>  }
>>  
>> +static char *nbd_dirname(BlockDriverState *bs, Error **errp)
>> +{
>> +    const char *path   = qdict_get_try_str(bs->options, "path");
>> +    const char *host   = qdict_get_try_str(bs->options, "host");
>> +    const char *port   = qdict_get_try_str(bs->options, "port");
>> +    const char *export = qdict_get_try_str(bs->options, "export");
>> +
>> +    if (path && export) {
>> +        return g_strdup_printf("nbd:unix:%s:exportname=%s/", path, export);
>> +    } else if (path && !export) {
>> +        return g_strdup_printf("nbd:unix:%s:exportname=", path);
>> +    } else if (!path && export && port) {
>> +        return g_strdup_printf("nbd://%s:%s/%s/", host, port, export);
>> +    } else if (!path && export && !port) {
>> +        return g_strdup_printf("nbd://%s/%s/", host, export);
>> +    } else if (!path && !export && port) {
>> +        return g_strdup_printf("nbd://%s:%s/", host, port);
>> +    } else if (!path && !export && !port) {
>> +        return g_strdup_printf("nbd://%s/", host);
>> +    }
> 
> Many different cases already, and it's only going to get worse.

Is it? I'm not sure we can support things like TLS here at all, so I
think this should be about it, even in the future.

>                                                                 Wouldn't
> it be better to store saddr in BDRVNBDState (so that we don't have to
> parse the options a second time here) and to go for the full version
> here always? Then it's one case for tcp and one for unix and that's it.

I think we can do that even without saddr, but I guess I need to see how
this patch interacts with my "blockdev-add for NBD" series anyway.

Thanks for the suggestion, I'll act on it,

Max

signature.asc
Description: OpenPGP digital signature