| Message ID | 20170116204921.31578-6-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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)
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(+)