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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3115 bytes --]

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>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]