| Message ID | 20200813162935.210070-12-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
On 13.08.20 18:29, Kevin Wolf wrote: > With this change, NBD exports are only created through the BlockExport > interface any more. This allows us finally to move things from the NBD > layer to the BlockExport layer if they make sense for other export > types, too. I see. > blk_exp_add() returns only a weak reference, so the explicit > nbd_export_put() goes away. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > include/block/export.h | 2 ++ > include/block/nbd.h | 1 + > block/export/export.c | 2 +- > blockdev-nbd.c | 8 +++++++- > qemu-nbd.c | 28 ++++++++++++++++++++++------ > 5 files changed, 33 insertions(+), 8 deletions(-) [...] > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index d5b084acc2..8dd127af52 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c [...] > @@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) > > assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); > > - if (!nbd_server) { > + if (!nbd_server && !is_qemu_nbd) { (This begs the question of how difficult it would be to let qemu-nbd use QMP’s nbd-server-start, but I will not ask it, for I fear the answer.) > error_setg(errp, "NBD server not running"); > return NULL; > } > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 48aa8a9d46..d967b8fcb9 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c [...] > @@ -1050,9 +1050,27 @@ int main(int argc, char **argv) > > bs->detect_zeroes = detect_zeroes; > > - export = nbd_export_new(bs, export_name, > - export_description, bitmap, readonly, shared > 1, > - writethrough, &error_fatal); > + nbd_server_is_qemu_nbd(true); > + > + export_opts = g_new(BlockExportOptions, 1); > + *export_opts = (BlockExportOptions) { > + .type = BLOCK_EXPORT_TYPE_NBD, > + .has_writethrough = true, > + .writethrough = writethrough, > + .u.nbd = { > + .device = g_strdup(bdrv_get_node_name(bs)), > + .has_name = true, > + .name = g_strdup(export_name), > + .has_description = !!export_description, > + .description = g_strdup(export_description), > + .has_writable = true, > + .writable = !readonly, > + .has_bitmap = !!bitmap, > + .bitmap = g_strdup(bitmap), > + }, > + }; > + blk_exp_add(export_opts, &error_fatal); Why not use the already-global qmp_block_export_add(), if we don’t need the return value here? (Will we require it at some point?) Max > + qapi_free_BlockExportOptions(export_opts); > > if (device) { > #if HAVE_NBD_DEVICE
On 17.08.20 16:27, Max Reitz wrote: > On 13.08.20 18:29, Kevin Wolf wrote: >> With this change, NBD exports are only created through the BlockExport >> interface any more. This allows us finally to move things from the NBD >> layer to the BlockExport layer if they make sense for other export >> types, too. > > I see. > >> blk_exp_add() returns only a weak reference, so the explicit >> nbd_export_put() goes away. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> include/block/export.h | 2 ++ >> include/block/nbd.h | 1 + >> block/export/export.c | 2 +- >> blockdev-nbd.c | 8 +++++++- >> qemu-nbd.c | 28 ++++++++++++++++++++++------ >> 5 files changed, 33 insertions(+), 8 deletions(-) > > [...] > >> diff --git a/blockdev-nbd.c b/blockdev-nbd.c >> index d5b084acc2..8dd127af52 100644 >> --- a/blockdev-nbd.c >> +++ b/blockdev-nbd.c > > [...] > >> @@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) >> >> assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); >> >> - if (!nbd_server) { >> + if (!nbd_server && !is_qemu_nbd) { > > (This begs the question of how difficult it would be to let qemu-nbd use > QMP’s nbd-server-start, but I will not ask it, for I fear the answer.) > >> error_setg(errp, "NBD server not running"); >> return NULL; >> } >> diff --git a/qemu-nbd.c b/qemu-nbd.c >> index 48aa8a9d46..d967b8fcb9 100644 >> --- a/qemu-nbd.c >> +++ b/qemu-nbd.c > > [...] > >> @@ -1050,9 +1050,27 @@ int main(int argc, char **argv) >> >> bs->detect_zeroes = detect_zeroes; >> >> - export = nbd_export_new(bs, export_name, >> - export_description, bitmap, readonly, shared > 1, >> - writethrough, &error_fatal); >> + nbd_server_is_qemu_nbd(true); >> + >> + export_opts = g_new(BlockExportOptions, 1); >> + *export_opts = (BlockExportOptions) { >> + .type = BLOCK_EXPORT_TYPE_NBD, >> + .has_writethrough = true, >> + .writethrough = writethrough, >> + .u.nbd = { >> + .device = g_strdup(bdrv_get_node_name(bs)), >> + .has_name = true, >> + .name = g_strdup(export_name), >> + .has_description = !!export_description, >> + .description = g_strdup(export_description), >> + .has_writable = true, >> + .writable = !readonly, >> + .has_bitmap = !!bitmap, >> + .bitmap = g_strdup(bitmap), >> + }, >> + }; >> + blk_exp_add(export_opts, &error_fatal); > > Why not use the already-global qmp_block_export_add(), if we don’t need > the return value here? (Will we require it at some point?) In the context of patch 13, which adds more blk_exp_* functions, it makes sense to make blk_exp_add() global, and then to use it here. So: Reviewed-by: Max Reitz <mreitz@redhat.com>
Am 17.08.2020 um 16:27 hat Max Reitz geschrieben: > On 13.08.20 18:29, Kevin Wolf wrote: > > With this change, NBD exports are only created through the BlockExport > > interface any more. This allows us finally to move things from the NBD > > layer to the BlockExport layer if they make sense for other export > > types, too. > > I see. > > > blk_exp_add() returns only a weak reference, so the explicit > > nbd_export_put() goes away. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > include/block/export.h | 2 ++ > > include/block/nbd.h | 1 + > > block/export/export.c | 2 +- > > blockdev-nbd.c | 8 +++++++- > > qemu-nbd.c | 28 ++++++++++++++++++++++------ > > 5 files changed, 33 insertions(+), 8 deletions(-) > > [...] > > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > > index d5b084acc2..8dd127af52 100644 > > --- a/blockdev-nbd.c > > +++ b/blockdev-nbd.c > > [...] > > > @@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) > > > > assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); > > > > - if (!nbd_server) { > > + if (!nbd_server && !is_qemu_nbd) { > > (This begs the question of how difficult it would be to let qemu-nbd use > QMP’s nbd-server-start, but I will not ask it, for I fear the answer.) (It would probably include something along the lines of "patches welcome". (I initially wanted to do this, but came to the conclusion that it wasn't for this series when I noticed the socket activation code and discussed with danpb on IRC how to integrate it in SocketAddress.)) > > error_setg(errp, "NBD server not running"); > > return NULL; > > } > > diff --git a/qemu-nbd.c b/qemu-nbd.c > > index 48aa8a9d46..d967b8fcb9 100644 > > --- a/qemu-nbd.c > > +++ b/qemu-nbd.c > > [...] > > > @@ -1050,9 +1050,27 @@ int main(int argc, char **argv) > > > > bs->detect_zeroes = detect_zeroes; > > > > - export = nbd_export_new(bs, export_name, > > - export_description, bitmap, readonly, shared > 1, > > - writethrough, &error_fatal); > > + nbd_server_is_qemu_nbd(true); > > + > > + export_opts = g_new(BlockExportOptions, 1); > > + *export_opts = (BlockExportOptions) { > > + .type = BLOCK_EXPORT_TYPE_NBD, > > + .has_writethrough = true, > > + .writethrough = writethrough, > > + .u.nbd = { > > + .device = g_strdup(bdrv_get_node_name(bs)), > > + .has_name = true, > > + .name = g_strdup(export_name), > > + .has_description = !!export_description, > > + .description = g_strdup(export_description), > > + .has_writable = true, > > + .writable = !readonly, > > + .has_bitmap = !!bitmap, > > + .bitmap = g_strdup(bitmap), > > + }, > > + }; > > + blk_exp_add(export_opts, &error_fatal); > > Why not use the already-global qmp_block_export_add(), if we don’t need > the return value here? (Will we require it at some point?) I can do that. I needed the return value originally, but after some reshuffling of the series it magically disappeared. Kevin
On 8/13/20 11:29 AM, Kevin Wolf wrote: > With this change, NBD exports are only created through the BlockExport > interface any more. This allows us finally to move things from the NBD s/are only/are now only/; s/any more // > layer to the BlockExport layer if they make sense for other export > types, too. > > blk_exp_add() returns only a weak reference, so the explicit > nbd_export_put() goes away. Feel free to rename get/put to ref/unref in your series if that makes life any easier. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > @@ -1050,9 +1050,27 @@ int main(int argc, char **argv) > > bs->detect_zeroes = detect_zeroes; > > - export = nbd_export_new(bs, export_name, > - export_description, bitmap, readonly, shared > 1, > - writethrough, &error_fatal); > + nbd_server_is_qemu_nbd(true); Feels a bit like a backdoor hack, but gets the job done (and as you said, we had quite an IRC conversation about what it would take to get socket activation working, so leaving that in qemu-nbd for now is reasonable for the first patch series). > + > + export_opts = g_new(BlockExportOptions, 1); > + *export_opts = (BlockExportOptions) { > + .type = BLOCK_EXPORT_TYPE_NBD, > + .has_writethrough = true, > + .writethrough = writethrough, > + .u.nbd = { > + .device = g_strdup(bdrv_get_node_name(bs)), > + .has_name = true, > + .name = g_strdup(export_name), > + .has_description = !!export_description, > + .description = g_strdup(export_description), > + .has_writable = true, > + .writable = !readonly, > + .has_bitmap = !!bitmap, > + .bitmap = g_strdup(bitmap), > + }, > + }; > + blk_exp_add(export_opts, &error_fatal); > + qapi_free_BlockExportOptions(export_opts); Looks sane.
diff --git a/include/block/export.h b/include/block/export.h index b1d7325403..5424bdc85d 100644 --- a/include/block/export.h +++ b/include/block/export.h @@ -29,4 +29,6 @@ struct BlockExport { extern const BlockExportDriver blk_exp_nbd; +BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp); + #endif diff --git a/include/block/nbd.h b/include/block/nbd.h index 50e1a46075..23030db3f1 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -350,6 +350,7 @@ void nbd_client_new(QIOChannelSocket *sioc, void nbd_client_get(NBDClient *client); void nbd_client_put(NBDClient *client); +void nbd_server_is_qemu_nbd(bool value); void nbd_server_start(SocketAddress *addr, const char *tls_creds, const char *tls_authz, uint32_t max_connections, Error **errp); diff --git a/block/export/export.c b/block/export/export.c index 2d5f92861c..12672228c7 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -36,7 +36,7 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type) return NULL; } -static BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) +BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) { const BlockExportDriver *drv; diff --git a/blockdev-nbd.c b/blockdev-nbd.c index d5b084acc2..8dd127af52 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -28,9 +28,15 @@ typedef struct NBDServerData { } NBDServerData; static NBDServerData *nbd_server; +static bool is_qemu_nbd; static void nbd_update_server_watch(NBDServerData *s); +void nbd_server_is_qemu_nbd(bool value) +{ + is_qemu_nbd = value; +} + static void nbd_blockdev_client_closed(NBDClient *client, bool ignored) { nbd_client_put(client); @@ -176,7 +182,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); - if (!nbd_server) { + if (!nbd_server && !is_qemu_nbd) { error_setg(errp, "NBD server not running"); return NULL; } diff --git a/qemu-nbd.c b/qemu-nbd.c index 48aa8a9d46..d967b8fcb9 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -65,7 +65,6 @@ #define MBR_SIZE 512 -static NBDExport *export; static int verbose; static char *srcpath; static SocketAddress *saddr; @@ -579,6 +578,7 @@ int main(int argc, char **argv) int old_stderr = -1; unsigned socket_activation; const char *pid_file_name = NULL; + BlockExportOptions *export_opts; /* The client thread uses SIGTERM to interrupt the server. A signal * handler ensures that "qemu-nbd -v -c" exits with a nice status code. @@ -1050,9 +1050,27 @@ int main(int argc, char **argv) bs->detect_zeroes = detect_zeroes; - export = nbd_export_new(bs, export_name, - export_description, bitmap, readonly, shared > 1, - writethrough, &error_fatal); + nbd_server_is_qemu_nbd(true); + + export_opts = g_new(BlockExportOptions, 1); + *export_opts = (BlockExportOptions) { + .type = BLOCK_EXPORT_TYPE_NBD, + .has_writethrough = true, + .writethrough = writethrough, + .u.nbd = { + .device = g_strdup(bdrv_get_node_name(bs)), + .has_name = true, + .name = g_strdup(export_name), + .has_description = !!export_description, + .description = g_strdup(export_description), + .has_writable = true, + .writable = !readonly, + .has_bitmap = !!bitmap, + .bitmap = g_strdup(bitmap), + }, + }; + blk_exp_add(export_opts, &error_fatal); + qapi_free_BlockExportOptions(export_opts); if (device) { #if HAVE_NBD_DEVICE @@ -1092,9 +1110,7 @@ int main(int argc, char **argv) do { main_loop_wait(false); if (state == TERMINATE) { - nbd_export_put(export); nbd_export_close_all(); - export = NULL; state = TERMINATED; } } while (state != TERMINATED);
With this change, NBD exports are only created through the BlockExport interface any more. This allows us finally to move things from the NBD layer to the BlockExport layer if they make sense for other export types, too. blk_exp_add() returns only a weak reference, so the explicit nbd_export_put() goes away. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- include/block/export.h | 2 ++ include/block/nbd.h | 1 + block/export/export.c | 2 +- blockdev-nbd.c | 8 +++++++- qemu-nbd.c | 28 ++++++++++++++++++++++------ 5 files changed, 33 insertions(+), 8 deletions(-)