diff mbox

[v2,07/19] qapi: Avoid use of 'data' member of qapi unions

Message ID 1456443528-13901-8-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Feb. 25, 2016, 11:38 p.m. UTC
qapi code generators currently create a 'void *data' member as
part of the anonymous union embedded in the C struct corresponding
to a qapi union.  However, directly assigning to this member of
the union feels a bit fishy, when we can directly use the rest
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(-)

Comments

Markus Armbruster March 2, 2016, 6:18 p.m. UTC | #1
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 mbox

Patch

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;