| Message ID | 1456443528-13901-8-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com> > --- > 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.
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); } @@ -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); } 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;