Re: [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits

[Markus Armbruster]

Here's how I'd rebase this on the simplified PATCH 06 I just posted as a
reply to yours:

>From e35449172828351f0acd85f236add45d3eb575a7 Mon Sep 17 00:00:00 2001
From: Eric Blake 
Date: Wed, 9 Mar 2016 17:55:28 -0700
Subject: [PATCH 07/14] qapi: Utilize implicit struct visits

Rather than generate inline per-member visits, take advantage
of the 'visit_type_FOO_members()' function for both event and
command marshalling.  This is possible now that implicit
structs can be visited like any other.

Generated code shrinks accordingly; events initialize a struct
based on parameters, through a new gen_param_var() helper, like:

|@@ -338,6 +250,9 @@ void qapi_event_send_block_job_error(con
| QMPEventFuncEmit emit = qmp_event_get_func_emit();
| QmpOutputVisitor *qov;
| Visitor *v;
|+q_obj_BLOCK_JOB_ERROR_arg param = {
|+(char *)device, operation, action
|+};
|
| if (!emit) {
| return;
@@ -351,19 +266,7 @@ void qapi_event_send_block_job_error(con
| if (err) {
| goto out;
| }
|-visit_type_str(v, "device", (char **), );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_IoOperationType(v, "operation", , );
|-if (err) {
|-goto out_obj;
|-}
|-visit_type_BlockErrorAction(v, "action", , );
|-if (err) {
|-goto out_obj;
|-}
|-out_obj:
|+visit_type_q_obj_BLOCK_JOB_ERROR_arg_members(v, , );
| visit_end_struct(v, err ? NULL : );

Notice that the initialization of 'param' has to cast away const
(just as the old gen_visit_members() had to do): we can't change
the signature of the user function (which uses 'const char *'), but
have to assign it to a non-const QAPI object (which requires
'char *').

Likewise, command marshalling generates call arguments from a
stack-allocated struct, rather than a list of local variables:

|@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb
| QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
| QapiDeallocVisitor *qdv;
| Visitor *v;
|-bool has_fdset_id = false;
|-int64_t fdset_id = 0;
|-bool has_opaque = false;
|-char *opaque = NULL;
|-
|-v = qmp_input_get_visitor(qiv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, );
|-if (err) {
|-goto out;
|-}
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , );
|-if (err) {
|-goto out;
|-}
|+q_obj_add_fd_arg qapi = {0};
|+
|+v = qmp_input_get_visitor(qiv);
|+visit_type_q_obj_add_fd_arg_members(v, , );
|+if (err) {
|+goto out;
| }
|
|-retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, );
|+retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
qapi.opaque, );
| if (err) {
| goto out;
| }
|@@ -88,12 +77,7 @@ out:
| qmp_input_visitor_cleanup(qiv);
| qdv = qapi_dealloc_visitor_new();
| v = qapi_dealloc_get_visitor(qdv);
|-if (visit_optional(v, "fdset-id", _fdset_id)) {
|-visit_type_int(v, "fdset-id", _id, NULL);
|-}
|-if (visit_optional(v, "opaque", _opaque)) {
|-visit_type_str(v, "opaque", , NULL);
|-}
|+visit_type_q_obj_add_fd_arg_members(v, , NULL);
| qapi_dealloc_visitor_cleanup(qdv);
| }

For the marshaller, it has the nice side effect of eliminating a
chance of collision between argument QMP names and local variables.

A similar collision still exists in the qapi_event_send_FOO().
Document it with a FIXME comment.

This patch also paves the way for some followup simplifications
in the generator, in subsequent patches.

Signed-off-by: Eric Blake 

Message-Id: <1457571335-10938-8-git-send-email-ebl...@redhat.com>
---
 scripts/qapi-commands.py | 28 
 scripts/qapi-event.py| 48 ++--
 2 files changed, 50 insertions(+), 26 deletions(-)

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 3784f33..5ffc381 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type):
 assert not arg_type.variants
 for memb in arg_type.members:
 if memb.optional:
-argstr += 'has_%s, ' % c_name(memb.name)
-argstr += '%s, ' % c_name(memb.name)
+argstr += 'qapi.has_%s, ' % c_name(memb.name)
+argstr += 'qapi.%s, ' % c_name(memb.name)
 
 lhs = ''
 if ret_type:
@@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type):
 QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
 QapiDeallocVisitor *qdv;
 Visitor *v;
-''')
+%(c_name)s qapi = {0};
 
-for memb in arg_type.members:
-if memb.optional:
-ret += mcgen('''
-bool has_%(c_name)s = false;
 ''',
- 

Re: [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits

[Eric Blake]

On 03/10/2016 12:05 PM, Markus Armbruster wrote:
> Eric Blake  writes:
> 
>> Rather than generate inline per-member visits, take advantage
>> of the 'visit_type_FOO_members()' function for both event and
>> command marshalling.  This is possible now that implicit
>> structs can be visited like any other.
>>

>> Likewise, command marshalling generates call arguments from a
>> stack-allocated struct, rather than a list of local variables:
>>

>> |-goto out;
>> |-}
>> |+q_obj_add_fd_arg qapi = {0};
> 
> Let's calls this arg.

Sure.

> 
>> |+
>> |+v = qmp_input_get_visitor(qiv);
>> |+visit_type_q_obj_add_fd_arg_members(v, , );
>> |+if (err) {
>> |+goto out;
>> | }
>> |
>> |-retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, );
>> |+retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
>> qapi.opaque, );

and this line then gets a bit shorter.

>> +++ b/scripts/qapi-event.py
>> @@ -28,6 +28,30 @@ def gen_event_send_decl(name, arg_type):
>>   proto=gen_event_send_proto(name, arg_type))

>> @@ -50,6 +74,7 @@ def gen_event_send(name, arg_type):
>>  QmpOutputVisitor *qov;
>>  Visitor *v;
>>  ''')
>> +ret += gen_param_var(arg_type)
>>
>>  ret += mcgen('''
>>

This is why I moved the blank line in 6/14.  But I can rearrange things
as you requested.

I'm also wondering if this should be split into two patches (one for
qapi-visit, one for qapi-commands); when I first started writing it, I
thought there would be some code sharing between the two with edits to
qapi.py (see the v4 posting); but now they are distinct enough that two
commits is just as easy to do.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits

[Markus Armbruster]

Eric Blake  writes:

> Rather than generate inline per-member visits, take advantage
> of the 'visit_type_FOO_members()' function for both event and
> command marshalling.  This is possible now that implicit
> structs can be visited like any other.
>
> Generated code shrinks accordingly; events initialize a struct
> based on parameters, through a new gen_param_var() helper, like:
>
> |@@ -338,6 +250,9 @@ void qapi_event_send_block_job_error(con
> | QMPEventFuncEmit emit = qmp_event_get_func_emit();
> | QmpOutputVisitor *qov;
> | Visitor *v;
> |+q_obj_BLOCK_JOB_ERROR_arg param = {
> |+(char *)device, operation, action
> |+};
> |
> | if (!emit) {
> | return;
> @@ -351,19 +266,7 @@ void qapi_event_send_block_job_error(con
> | if (err) {
> | goto out;
> | }
> |-visit_type_str(v, "device", (char **), );
> |-if (err) {
> |-goto out_obj;
> |-}
> |-visit_type_IoOperationType(v, "operation", , );
> |-if (err) {
> |-goto out_obj;
> |-}
> |-visit_type_BlockErrorAction(v, "action", , );
> |-if (err) {
> |-goto out_obj;
> |-}
> |-out_obj:
> |+visit_type_q_obj_BLOCK_JOB_ERROR_arg_members(v, , );
> | visit_end_struct(v, err ? NULL : );
>
> Notice that the initialization of 'param' has to cast away const
> (just as the old gen_visit_members() had to do): we can't change
> the signature of the user function (which uses 'const char *'), but
> have to assign it to a non-const QAPI object (which requires
> 'char *').
>
> Likewise, command marshalling generates call arguments from a
> stack-allocated struct, rather than a list of local variables:
>
> |@@ -57,26 +57,15 @@ void qmp_marshal_add_fd(QDict *args, QOb
> | QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
> | QapiDeallocVisitor *qdv;
> | Visitor *v;
> |-bool has_fdset_id = false;
> |-int64_t fdset_id = 0;
> |-bool has_opaque = false;
> |-char *opaque = NULL;
> |-
> |-v = qmp_input_get_visitor(qiv);
> |-if (visit_optional(v, "fdset-id", _fdset_id)) {
> |-visit_type_int(v, "fdset-id", _id, );
> |-if (err) {
> |-goto out;
> |-}
> |-}
> |-if (visit_optional(v, "opaque", _opaque)) {
> |-visit_type_str(v, "opaque", , );
> |-if (err) {
> |-goto out;
> |-}
> |+q_obj_add_fd_arg qapi = {0};

Let's calls this arg.

> |+
> |+v = qmp_input_get_visitor(qiv);
> |+visit_type_q_obj_add_fd_arg_members(v, , );
> |+if (err) {
> |+goto out;
> | }
> |
> |-retval = qmp_add_fd(has_fdset_id, fdset_id, has_opaque, opaque, );
> |+retval = qmp_add_fd(qapi.has_fdset_id, qapi.fdset_id, qapi.has_opaque, 
> qapi.opaque, );
> | if (err) {
> | goto out;
> | }
> |@@ -88,12 +77,7 @@ out:
> | qmp_input_visitor_cleanup(qiv);
> | qdv = qapi_dealloc_visitor_new();
> | v = qapi_dealloc_get_visitor(qdv);
> |-if (visit_optional(v, "fdset-id", _fdset_id)) {
> |-visit_type_int(v, "fdset-id", _id, NULL);
> |-}
> |-if (visit_optional(v, "opaque", _opaque)) {
> |-visit_type_str(v, "opaque", , NULL);
> |-}
> |+visit_type_q_obj_add_fd_arg_members(v, , NULL);
> | qapi_dealloc_visitor_cleanup(qdv);
> | }
>
> For the marshaller, it has the nice side effect of eliminating a
> chance of collision between argument QMP names and local variables.
>
> This patch also paves the way for some followup simplifications
> in the generator, in subsequent patches.
>
> Signed-off-by: Eric Blake 
>
> ---
> v5: move qapi.py:gen_struct_init() to qapi-event.py:gen_param_var(),
> improve commit message
> v4: new patch
> ---
>  scripts/qapi-commands.py | 28 
>  scripts/qapi-event.py| 40 +++-
>  2 files changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 3784f33..5ffc381 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -33,8 +33,8 @@ def gen_call(name, arg_type, ret_type):
>  assert not arg_type.variants
>  for memb in arg_type.members:
>  if memb.optional:
> -argstr += 'has_%s, ' % c_name(memb.name)
> -argstr += '%s, ' % c_name(memb.name)
> +argstr += 'qapi.has_%s, ' % c_name(memb.name)
> +argstr += 'qapi.%s, ' % c_name(memb.name)
>
>  lhs = ''
>  if ret_type:
> @@ -71,21 +71,10 @@ def gen_marshal_vars(arg_type, ret_type):
>  QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
>  QapiDeallocVisitor *qdv;
>  Visitor *v;
> -''')
> +%(c_name)s qapi = {0};
>
> -for memb in arg_type.members:
> -if memb.optional:
> -ret += mcgen('''
> -bool has_%(c_name)s = false;
>  ''',
> - c_name=c_name(memb.name))
>