diff mbox series

[RFC,11/22] qemu-nbd: Use blk_exp_add() to create the export

Message ID 20200813162935.210070-12-kwolf@redhat.com
State New, archived
Headers show
Series block/export: Add infrastructure and QAPI for block exports | expand

Commit Message

Kevin Wolf Aug. 13, 2020, 4:29 p.m. UTC
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(-)

Comments

Max Reitz Aug. 17, 2020, 2:27 p.m. UTC | #1
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
Max Reitz Aug. 17, 2020, 2:38 p.m. UTC | #2
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>
Kevin Wolf Aug. 17, 2020, 3:01 p.m. UTC | #3
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
Eric Blake Aug. 19, 2020, 8:35 p.m. UTC | #4
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 mbox series

Patch

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);