[v4,13/25] block/nbd: Implement bdrv_dirname()
diff mbox

Message ID 20170116204921.31578-6-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Jan. 16, 2017, 8:49 p.m. UTC
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 <mreitz@redhat.com>
---
 block/nbd.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Eric Blake Jan. 16, 2017, 11:21 p.m. UTC | #1
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 <mreitz@redhat.com>
> ---
>  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)?

> +    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.
Max Reitz Jan. 18, 2017, 1:37 p.m. UTC | #2
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 <mreitz@redhat.com>
>> ---
>>  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

Patch
diff mbox

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;
+    }
+
+    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;
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -569,6 +597,7 @@  static BlockDriver bdrv_nbd = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_dirname               = nbd_dirname,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -588,6 +617,7 @@  static BlockDriver bdrv_nbd_tcp = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_dirname               = nbd_dirname,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -607,6 +637,7 @@  static BlockDriver bdrv_nbd_unix = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_dirname               = nbd_dirname,
 };
 
 static void bdrv_nbd_init(void)