[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 07/19] qapi: Avoid use of 'data' member of qa
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 07/19] qapi: Avoid use of 'data' member of qapi unions |
Date: |
Wed, 02 Mar 2016 19:18:43 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> qapi code generators currently create a 'void *data' member as
QAPI
> part of the anonymous union embedded in the C struct corresponding
> to a qapi union. However, directly assigning to this member of
QAPI
> the union feels a bit fishy, when we can directly use the rest
Suggest to drop "directly", or perhaps say "when we can assign to
another member
> of the struct instead.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Daniel P. Berrange <address@hidden>
> ---
> v2: add R-b
> v1: no change
> Previously posted as part of qapi cleanup series F:
> v6: rebase to latest
> ---
> blockdev.c | 31 +++++++++++++++++--------------
> ui/input.c | 2 +-
> 2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index d4bc435..0f20c65 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1202,15 +1202,11 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
> }
> }
>
> -static void blockdev_do_action(TransactionActionKind type, void *data,
> - Error **errp)
> +static void blockdev_do_action(TransactionAction *action, Error **errp)
> {
> - TransactionAction action;
> TransactionActionList list;
>
> - action.type = type;
> - action.u.data = data;
> - list.value = &action;
> + list.value = action;
> list.next = NULL;
> qmp_transaction(&list, false, NULL, errp);
> }
Here, you avoid use of data by assigning the whole struct instead of its
members.
> @@ -1236,8 +1232,11 @@ void qmp_blockdev_snapshot_sync(bool has_device, const
> char *device,
> .has_mode = has_mode,
> .mode = mode,
> };
> - blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
> - &snapshot, errp);
> + TransactionAction action = {
> + .type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC,
> + .u.blockdev_snapshot_sync = &snapshot,
> + };
> + blockdev_do_action(&action, errp);
> }
>
However, the call sites become wordier. I guess avoiding type-punning
is worth a bit of verbosity.
> void qmp_blockdev_snapshot(const char *node, const char *overlay,
> @@ -1247,9 +1246,11 @@ void qmp_blockdev_snapshot(const char *node, const
> char *overlay,
> .node = (char *) node,
> .overlay = (char *) overlay
> };
> -
> - blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
> - &snapshot_data, errp);
> + TransactionAction action = {
> + .type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT,
> + .u.blockdev_snapshot = &snapshot_data,
> + };
> + blockdev_do_action(&action, errp);
> }
>
> void qmp_blockdev_snapshot_internal_sync(const char *device,
> @@ -1260,9 +1261,11 @@ void qmp_blockdev_snapshot_internal_sync(const char
> *device,
> .device = (char *) device,
> .name = (char *) name
> };
> -
> -
> blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
> - &snapshot, errp);
> + TransactionAction action = {
> + .type = TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
> + .u.blockdev_snapshot_internal_sync = &snapshot,
> + };
> + blockdev_do_action(&action, errp);
> }
>
> SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
> diff --git a/ui/input.c b/ui/input.c
> index e15c618..1e81c25 100644
> --- a/ui/input.c
> +++ b/ui/input.c
> @@ -472,7 +472,7 @@ InputEvent *qemu_input_event_new_move(InputEventKind kind,
> InputMoveEvent *move = g_new0(InputMoveEvent, 1);
>
> evt->type = kind;
> - evt->u.data = move;
> + evt->u.rel = move; /* also would work as evt->u.abs */
> move->axis = axis;
> move->value = value;
> return evt;
Suggest to say /* evt->u.rel is the same as evt.u.abs */
Can't think of a way to build-assert that.
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v2 07/19] qapi: Avoid use of 'data' member of qapi unions,
Markus Armbruster <=