| Message ID | 1461706338-20219-17-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mreitz@redhat.com> > --- > 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. 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. Kevin
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 <mreitz@redhat.com> >> --- >> 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
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); + } + + 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", @@ -487,6 +513,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 = { @@ -507,6 +534,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 = { @@ -527,6 +555,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 | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)