diff mbox series

[v5,03/12] nbd: Utilize QAPI_CLONE for type conversion

Message ID 20201023183652.478921-4-eblake@redhat.com
State Superseded
Headers show
Series Exposing backing-chain allocation over NBD | expand

Commit Message

Eric Blake Oct. 23, 2020, 6:36 p.m. UTC
Rather than open-coding the translation from the deprecated
NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's
better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:
first, if we do any more refactoring of the base type (which an
upcoming patch plans to do), we don't have to revisit the open-coding.
Second, our assignment to arg->name is fishy: the generated QAPI code
currently does not visit it if arg->has_name is false, but if it DID
visit it, we would have introduced a double-free situation when arg is
finally freed.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 blockdev-nbd.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 24, 2020, 9:17 a.m. UTC | #1
23.10.2020 21:36, Eric Blake wrote:
> Rather than open-coding the translation from the deprecated

> NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's

> better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:

> first, if we do any more refactoring of the base type (which an

> upcoming patch plans to do), we don't have to revisit the open-coding.

> Second, our assignment to arg->name is fishy: the generated QAPI code

> currently does not visit it if arg->has_name is false, but if it DID

> visit it, we would have introduced a double-free situation when arg is

> finally freed.

> 

> Signed-off-by: Eric Blake<eblake@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir
Markus Armbruster Oct. 26, 2020, 2:41 p.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> Rather than open-coding the translation from the deprecated

> NbdServerAddOptions type to the preferred BlockExportOptionsNbd, it's

> better to utilize QAPI_CLONE_MEMBERS.  This solves a couple of issues:

> first, if we do any more refactoring of the base type (which an

> upcoming patch plans to do), we don't have to revisit the open-coding.

> Second, our assignment to arg->name is fishy: the generated QAPI code

> currently does not visit it if arg->has_name is false, but if it DID

> visit it, we would have introduced a double-free situation when arg is

> finally freed.

>

> Signed-off-by: Eric Blake <eblake@redhat.com>

> ---

>  blockdev-nbd.c | 15 ++++++---------

>  1 file changed, 6 insertions(+), 9 deletions(-)

>

> diff --git a/blockdev-nbd.c b/blockdev-nbd.c

> index 8174023e5c47..cee9134b12eb 100644

> --- a/blockdev-nbd.c

> +++ b/blockdev-nbd.c

> @@ -14,6 +14,8 @@

>  #include "sysemu/block-backend.h"

>  #include "hw/block/block.h"

>  #include "qapi/error.h"

> +#include "qapi/clone-visitor.h"

> +#include "qapi/qapi-visit-block-export.h"

>  #include "qapi/qapi-commands-block-export.h"

>  #include "block/nbd.h"

>  #include "io/channel-socket.h"

> @@ -195,7 +197,8 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)

>       * the device name as a default here for compatibility.

>       */

>      if (!arg->has_name) {

> -        arg->name = arg->device;

> +        arg->has_name = true;

> +        arg->name = g_strdup(arg->device);

>      }


This is the fix you mentioned in the commit message.

>

>      export_opts = g_new(BlockExportOptions, 1);

> @@ -205,15 +208,9 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)

       *export_opts = (BlockExportOptions) {
           .type                   = BLOCK_EXPORT_TYPE_NBD,
           .id                     = g_strdup(arg->name),
>          .node_name              = g_strdup(bdrv_get_node_name(bs)),

>          .has_writable           = arg->has_writable,

>          .writable               = arg->writable,


Explicit initialization of all the common members, except for
@writethrough.  @writethrough is optional, so not mentioning it makes it
absent.  I don't mind.

> -        .u.nbd = {

> -            .has_name           = true,

> -            .name               = g_strdup(arg->name),

> -            .has_description    = arg->has_description,

> -            .description        = g_strdup(arg->description),

> -            .has_bitmap         = arg->has_bitmap,

> -            .bitmap             = g_strdup(arg->bitmap),


Explicit initialization of all the variant members: copy of @arg.

> -        },

>      };

> +    QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, &export_opts->u.nbd,

> +                       qapi_NbdServerAddOptions_base(arg));


Another (and better) way to copy.

>

>      /*

>       * nbd-server-add doesn't complain when a read-only device should be


Reviewed-by: Markus Armbruster <armbru@redhat.com>
diff mbox series

Patch

diff --git a/blockdev-nbd.c b/blockdev-nbd.c
index 8174023e5c47..cee9134b12eb 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -14,6 +14,8 @@ 
 #include "sysemu/block-backend.h"
 #include "hw/block/block.h"
 #include "qapi/error.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-block-export.h"
 #include "qapi/qapi-commands-block-export.h"
 #include "block/nbd.h"
 #include "io/channel-socket.h"
@@ -195,7 +197,8 @@  void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
      * the device name as a default here for compatibility.
      */
     if (!arg->has_name) {
-        arg->name = arg->device;
+        arg->has_name = true;
+        arg->name = g_strdup(arg->device);
     }

     export_opts = g_new(BlockExportOptions, 1);
@@ -205,15 +208,9 @@  void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp)
         .node_name              = g_strdup(bdrv_get_node_name(bs)),
         .has_writable           = arg->has_writable,
         .writable               = arg->writable,
-        .u.nbd = {
-            .has_name           = true,
-            .name               = g_strdup(arg->name),
-            .has_description    = arg->has_description,
-            .description        = g_strdup(arg->description),
-            .has_bitmap         = arg->has_bitmap,
-            .bitmap             = g_strdup(arg->bitmap),
-        },
     };
+    QAPI_CLONE_MEMBERS(BlockExportOptionsNbd, &export_opts->u.nbd,
+                       qapi_NbdServerAddOptions_base(arg));

     /*
      * nbd-server-add doesn't complain when a read-only device should be