Eric Blake <ebl...@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 <ebl...@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 <arm...@redhat.com>
