| Message ID | 20201023183652.478921-4-eblake@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
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
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 --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
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(-)