diff mbox series

[RFC,03/21] qapi: New classes QAPIGenC, QAPIGenH, QAPIGenDoc

Message ID 20180202130336.24719-4-armbru@redhat.com
State New
Headers show
Series Modularize generated QAPI code | expand

Commit Message

Markus Armbruster Feb. 2, 2018, 1:03 p.m. UTC
These classes encapsulate accumulating and writing output.

Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
rather shallow: most of the output accumulation is not converted.
Left for later.

The indentation machinery uses a single global variable indent_level,
even though we generally interleave creation of a .c and its .h.  It
should become instance variable of QAPIGenC.  Also left for later.

Documentation generation isn't converted, and QAPIGenDoc isn't used.
This will change shortly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi-commands.py   | 27 ++++++-------
 scripts/qapi-event.py      | 26 +++++++------
 scripts/qapi-introspect.py | 22 ++++++-----
 scripts/qapi-types.py      | 26 +++++++------
 scripts/qapi-visit.py      | 26 +++++++------
 scripts/qapi.py            | 96 ++++++++++++++++++++++++++--------------------
 6 files changed, 122 insertions(+), 101 deletions(-)

Comments

Eric Blake Feb. 2, 2018, 3:59 p.m. UTC | #1
On 02/02/2018 07:03 AM, Markus Armbruster wrote:
> These classes encapsulate accumulating and writing output.
> 
> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
> rather shallow: most of the output accumulation is not converted.
> Left for later.
> 
> The indentation machinery uses a single global variable indent_level,
> even though we generally interleave creation of a .c and its .h.  It
> should become instance variable of QAPIGenC.  Also left for later.
> 
> Documentation generation isn't converted, and QAPIGenDoc isn't used.
> This will change shortly.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py   | 27 ++++++-------
>  scripts/qapi-event.py      | 26 +++++++------
>  scripts/qapi-introspect.py | 22 ++++++-----
>  scripts/qapi-types.py      | 26 +++++++------
>  scripts/qapi-visit.py      | 26 +++++++------
>  scripts/qapi.py            | 96 ++++++++++++++++++++++++++--------------------
>  6 files changed, 122 insertions(+), 101 deletions(-)
> 

A little bit longer due to more structure, but reasonable diffstat in
that it shows the conversion is fairly straightforward and opens the
doors for later patches to use the new structures more effectively.

>  
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenEventVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenEventVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)

I don't know if it is worth a sentence in the commit message that the
visitor variable is renamed from 'gen' to 'vis' for less confusion with
the new class instances 'genc' and 'genh'.

> +++ b/scripts/qapi-types.py
> @@ -180,7 +180,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self.decl = ''
>          self.defn = ''
>          self._fwdecl = ''
> -        self._btin = guardstart('QAPI_TYPES_BUILTIN')
> +        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')

Tweaks like this means you were paying attention to still producing
identical generated files; always a good sign.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Feb. 3, 2018, 8:49 a.m. UTC | #2
Eric Blake <eblake@redhat.com> writes:

> On 02/02/2018 07:03 AM, Markus Armbruster wrote:
>> These classes encapsulate accumulating and writing output.
>> 
>> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
>> rather shallow: most of the output accumulation is not converted.
>> Left for later.
>> 
>> The indentation machinery uses a single global variable indent_level,
>> even though we generally interleave creation of a .c and its .h.  It
>> should become instance variable of QAPIGenC.  Also left for later.
>> 
>> Documentation generation isn't converted, and QAPIGenDoc isn't used.
>> This will change shortly.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-commands.py   | 27 ++++++-------
>>  scripts/qapi-event.py      | 26 +++++++------
>>  scripts/qapi-introspect.py | 22 ++++++-----
>>  scripts/qapi-types.py      | 26 +++++++------
>>  scripts/qapi-visit.py      | 26 +++++++------
>>  scripts/qapi.py            | 96 ++++++++++++++++++++++++++--------------------
>>  6 files changed, 122 insertions(+), 101 deletions(-)
>> 
>
> A little bit longer due to more structure, but reasonable diffstat in
> that it shows the conversion is fairly straightforward and opens the
> doors for later patches to use the new structures more effectively.
>
>>  
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenEventVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenEventVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>
> I don't know if it is worth a sentence in the commit message that the
> visitor variable is renamed from 'gen' to 'vis' for less confusion with
> the new class instances 'genc' and 'genh'.

Did the rename give you pause when reviewing?

>> +++ b/scripts/qapi-types.py
>> @@ -180,7 +180,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>          self.decl = ''
>>          self.defn = ''
>>          self._fwdecl = ''
>> -        self._btin = guardstart('QAPI_TYPES_BUILTIN')
>> +        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
>
> Tweaks like this means you were paying attention to still producing
> identical generated files; always a good sign.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
Marc-Andre Lureau Feb. 5, 2018, 1:44 p.m. UTC | #3
Hi

On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
> These classes encapsulate accumulating and writing output.
>
> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
> rather shallow: most of the output accumulation is not converted.
> Left for later.
>
> The indentation machinery uses a single global variable indent_level,
> even though we generally interleave creation of a .c and its .h.  It
> should become instance variable of QAPIGenC.  Also left for later.
>
> Documentation generation isn't converted, and QAPIGenDoc isn't used.
> This will change shortly.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi-commands.py   | 27 ++++++-------
>  scripts/qapi-event.py      | 26 +++++++------
>  scripts/qapi-introspect.py | 22 ++++++-----
>  scripts/qapi-types.py      | 26 +++++++------
>  scripts/qapi-visit.py      | 26 +++++++------
>  scripts/qapi.py            | 96 ++++++++++++++++++++++++++--------------------
>  6 files changed, 122 insertions(+), 101 deletions(-)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index a861ac52e7..4be7dbc482 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -260,12 +260,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP commands
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qmp-marshal.c', 'qmp-commands.h',
> -                            blurb, __doc__)
> -
> -fdef.write(mcgen('''
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qemu/module.h"
> @@ -279,21 +277,24 @@ fdef.write(mcgen('''
>  #include "%(prefix)sqmp-commands.h"
>
>  ''',
> -                 prefix=prefix))
> +                prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "%(prefix)sqapi-types.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/dispatch.h"
>
>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>  ''',
> -                  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
> +                prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenCommandVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenCommandVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qmp-marshal.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qmp-commands.h')
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index b1d611c5ea..da3de17c76 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -176,11 +176,10 @@ blurb = '''
>   * Schema-defined QAPI/QMP events
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qapi-event.c', 'qapi-event.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "%(prefix)sqapi-event.h"
> @@ -190,22 +189,25 @@ fdef.write(mcgen('''
>  #include "qapi/qmp-event.h"
>
>  ''',
> -                 prefix=prefix))
> +                prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "qapi/util.h"
>  #include "qapi/qmp/qdict.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> -                  prefix=prefix))
> +                prefix=prefix))
>
>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenEventVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenEventVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qapi-event.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qapi-event.h')
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index bd9253a172..c654f8fa94 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -181,21 +181,23 @@ blurb = '''
>   * QAPI/QMP schema introspection
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qmp-introspect.c', 'qmp-introspect.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "%(prefix)sqmp-introspect.h"
>
>  ''',
> -                 prefix=prefix))
> +                prefix=prefix))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qmp-introspect.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qmp-introspect.h')
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 1103dbda2d..97406b3368 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -180,7 +180,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>          self.decl = ''
>          self.defn = ''
>          self._fwdecl = ''
> -        self._btin = guardstart('QAPI_TYPES_BUILTIN')
> +        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
>
>      def visit_end(self):
>          self.decl = self._fwdecl + self.decl
> @@ -256,26 +256,28 @@ blurb = '''
>   * Schema-defined QAPI types
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qapi-types.c', 'qapi-types.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qapi/dealloc-visitor.h"
>  #include "%(prefix)sqapi-types.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                 prefix=prefix))
> +                prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "qapi/util.h"
>  '''))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenTypeVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenTypeVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qapi-types.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qapi-types.h')
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 5231f89c36..d1b25daf0d 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -339,30 +339,32 @@ blurb = '''
>   * Schema-defined QAPI visitors
>  '''
>
> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
> -                            'qapi-visit.c', 'qapi-visit.h',
> -                            blurb, __doc__)
> +genc = QAPIGenC(blurb, __doc__)
> +genh = QAPIGenH(blurb, __doc__)
>
> -fdef.write(mcgen('''
> +genc.body(mcgen('''
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "qapi/error.h"
>  #include "%(prefix)sqapi-visit.h"
>  ''',
> -                 prefix=prefix))
> +                prefix=prefix))
>
> -fdecl.write(mcgen('''
> +genh.body(mcgen('''
>  #include "qapi/visitor.h"
>  #include "qapi/qmp/qerror.h"
>  #include "%(prefix)sqapi-types.h"
>
>  ''',
> -                  prefix=prefix))
> +                prefix=prefix))
>
>  schema = QAPISchema(input_file)
> -gen = QAPISchemaGenVisitVisitor()
> -schema.visit(gen)
> -fdef.write(gen.defn)
> -fdecl.write(gen.decl)
> +vis = QAPISchemaGenVisitVisitor()
> +schema.visit(vis)
> +genc.body(vis.defn)
> +genh.body(vis.decl)
>
> -close_output(fdef, fdecl)
> +if do_c:
> +    genc.write(output_dir, prefix + 'qapi-visit.c')
> +if do_h:
> +    genh.write(output_dir, prefix + 'qapi-visit.h')
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index d0816f7479..d73ef618e2 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -2,7 +2,7 @@
>  # QAPI helper library
>  #
>  # Copyright IBM, Corp. 2011
> -# Copyright (c) 2013-2016 Red Hat Inc.
> +# Copyright (c) 2013-2018 Red Hat Inc.
>  #
>  # Authors:
>  #  Anthony Liguori <aliguori@us.ibm.com>
> @@ -1820,7 +1820,6 @@ def guardname(filename):
>
>  def guardstart(name):
>      return mcgen('''
> -
>  #ifndef %(name)s
>  #define %(name)s
>
> @@ -1832,7 +1831,6 @@ def guardend(name):
>      return mcgen('''
>
>  #endif /* %(name)s */
> -
>  ''',
>                   name=guardname(name))
>
> @@ -1970,17 +1968,53 @@ def parse_command_line(extra_options='', extra_long_options=[]):
>
>      return (fname, output_dir, do_c, do_h, prefix, extra_opts)
>
> +
>  #
> -# Generate output files with boilerplate
> +# Accumulate and write output
>  #
>
> +class QAPIGen(object):
> +
> +    def __init__(self):
> +        self._preamble = ''
> +        self._body = ''
> +
> +    def preamble(self, text):
> +        self._preamble += text
> +
> +    def body(self, text):
> +        self._body += text
> +
> +    def top(self, fname):
> +        return ''
> +
> +    def bottom(self, fname):
> +        return ''
> +

Some methods appends, other return. That's a bit confusing. Why not
name them accordingly? add_preamble, add_body, get_top...?

> +    def write(self, output_dir, fname):
> +        if output_dir:
> +            try:
> +                os.makedirs(output_dir)
> +            except os.error as e:
> +                if e.errno != errno.EEXIST:
> +                    raise
> +        f = open(os.path.join(output_dir, fname), 'w')
> +        f.write(self.top(fname) + self._preamble + self._body
> +                + self.bottom(fname))
> +        f.close()
> +
> +
> +class QAPIGenC(QAPIGen):
> +
> +    def __init__(self, blurb, pydoc):
> +        QAPIGen.__init__(self)
> +        self._blurb = blurb.strip('\n')
> +        self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
> +                                                  re.MULTILINE))
>
> -def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
> -    guard = guardname(prefix + h_file)
> -    c_file = output_dir + prefix + c_file
> -    h_file = output_dir + prefix + h_file
> -    copyright = '\n * '.join(re.findall(r'^Copyright .*', doc, re.MULTILINE))
> -    comment = mcgen('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +    def top(self, fname):
> +        return mcgen('''
> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>
>  /*
>  %(blurb)s
> @@ -1992,41 +2026,19 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
>   */
>
>  ''',
> -                    blurb=blurb.strip('\n'), copyright=copyright)
> +                     blurb=self._blurb, copyright=self._copyright)
>
> -    if output_dir:
> -        try:
> -            os.makedirs(output_dir)
> -        except os.error as e:
> -            if e.errno != errno.EEXIST:
> -                raise
>
> -    def maybe_open(really, name, opt):
> -        if really:
> -            return open(name, opt)
> -        else:
> -            import StringIO
> -            return StringIO.StringIO()
> +class QAPIGenH(QAPIGenC):
>
> -    fdef = maybe_open(do_c, c_file, 'w')
> -    fdecl = maybe_open(do_h, h_file, 'w')
> +    def top(self, fname):
> +        return QAPIGenC.top(self, fname) + guardstart(fname)
>
> -    fdef.write(comment)
> -    fdecl.write(comment)
> -    fdecl.write(mcgen('''
> -#ifndef %(guard)s
> -#define %(guard)s
> +    def bottom(self, fname):
> +        return guardend(fname)
>
> -''',
> -                      guard=guard))
>
> -    return (fdef, fdecl)
> -
> -
> -def close_output(fdef, fdecl):
> -    fdecl.write(mcgen('''
> -
> -#endif
> -'''))
> -    fdecl.close()
> -    fdef.close()
> +class QAPIGenDoc(QAPIGen):
> +    def top(self, fname):
> +        return (QAPIGen.top(self, fname)
> +                + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
> --
> 2.13.6
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Markus Armbruster Feb. 5, 2018, 3:34 p.m. UTC | #4
Marc-Andre Lureau <mlureau@redhat.com> writes:

> Hi
>
> On Fri, Feb 2, 2018 at 2:03 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> These classes encapsulate accumulating and writing output.
>>
>> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
>> rather shallow: most of the output accumulation is not converted.
>> Left for later.
>>
>> The indentation machinery uses a single global variable indent_level,
>> even though we generally interleave creation of a .c and its .h.  It
>> should become instance variable of QAPIGenC.  Also left for later.
>>
>> Documentation generation isn't converted, and QAPIGenDoc isn't used.
>> This will change shortly.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi-commands.py   | 27 ++++++-------
>>  scripts/qapi-event.py      | 26 +++++++------
>>  scripts/qapi-introspect.py | 22 ++++++-----
>>  scripts/qapi-types.py      | 26 +++++++------
>>  scripts/qapi-visit.py      | 26 +++++++------
>>  scripts/qapi.py            | 96 ++++++++++++++++++++++++++--------------------
>>  6 files changed, 122 insertions(+), 101 deletions(-)
>>
>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> index a861ac52e7..4be7dbc482 100644
>> --- a/scripts/qapi-commands.py
>> +++ b/scripts/qapi-commands.py
>> @@ -260,12 +260,10 @@ blurb = '''
>>   * Schema-defined QAPI/QMP commands
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qmp-marshal.c', 'qmp-commands.h',
>> -                            blurb, __doc__)
>> -
>> -fdef.write(mcgen('''
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qemu/module.h"
>> @@ -279,21 +277,24 @@ fdef.write(mcgen('''
>>  #include "%(prefix)sqmp-commands.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "%(prefix)sqapi-types.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "qapi/qmp/dispatch.h"
>>
>>  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>>  ''',
>> -                  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>> +                prefix=prefix, c_prefix=c_name(prefix, protect=False)))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenCommandVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenCommandVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qmp-marshal.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qmp-commands.h')
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index b1d611c5ea..da3de17c76 100644
>> --- a/scripts/qapi-event.py
>> +++ b/scripts/qapi-event.py
>> @@ -176,11 +176,10 @@ blurb = '''
>>   * Schema-defined QAPI/QMP events
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qapi-event.c', 'qapi-event.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "%(prefix)sqapi-event.h"
>> @@ -190,22 +189,25 @@ fdef.write(mcgen('''
>>  #include "qapi/qmp-event.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/util.h"
>>  #include "qapi/qmp/qdict.h"
>>  #include "%(prefix)sqapi-types.h"
>>
>>  ''',
>> -                  prefix=prefix))
>> +                prefix=prefix))
>>
>>  event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenEventVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenEventVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qapi-event.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qapi-event.h')
>> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
>> index bd9253a172..c654f8fa94 100644
>> --- a/scripts/qapi-introspect.py
>> +++ b/scripts/qapi-introspect.py
>> @@ -181,21 +181,23 @@ blurb = '''
>>   * QAPI/QMP schema introspection
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qmp-introspect.c', 'qmp-introspect.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "%(prefix)sqmp-introspect.h"
>>
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qmp-introspect.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qmp-introspect.h')
>> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
>> index 1103dbda2d..97406b3368 100644
>> --- a/scripts/qapi-types.py
>> +++ b/scripts/qapi-types.py
>> @@ -180,7 +180,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
>>          self.decl = ''
>>          self.defn = ''
>>          self._fwdecl = ''
>> -        self._btin = guardstart('QAPI_TYPES_BUILTIN')
>> +        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
>>
>>      def visit_end(self):
>>          self.decl = self._fwdecl + self.decl
>> @@ -256,26 +256,28 @@ blurb = '''
>>   * Schema-defined QAPI types
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qapi-types.c', 'qapi-types.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qapi/dealloc-visitor.h"
>>  #include "%(prefix)sqapi-types.h"
>>  #include "%(prefix)sqapi-visit.h"
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/util.h"
>>  '''))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenTypeVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenTypeVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qapi-types.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qapi-types.h')
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index 5231f89c36..d1b25daf0d 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -339,30 +339,32 @@ blurb = '''
>>   * Schema-defined QAPI visitors
>>  '''
>>
>> -(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
>> -                            'qapi-visit.c', 'qapi-visit.h',
>> -                            blurb, __doc__)
>> +genc = QAPIGenC(blurb, __doc__)
>> +genh = QAPIGenH(blurb, __doc__)
>>
>> -fdef.write(mcgen('''
>> +genc.body(mcgen('''
>>  #include "qemu/osdep.h"
>>  #include "qemu-common.h"
>>  #include "qapi/error.h"
>>  #include "%(prefix)sqapi-visit.h"
>>  ''',
>> -                 prefix=prefix))
>> +                prefix=prefix))
>>
>> -fdecl.write(mcgen('''
>> +genh.body(mcgen('''
>>  #include "qapi/visitor.h"
>>  #include "qapi/qmp/qerror.h"
>>  #include "%(prefix)sqapi-types.h"
>>
>>  ''',
>> -                  prefix=prefix))
>> +                prefix=prefix))
>>
>>  schema = QAPISchema(input_file)
>> -gen = QAPISchemaGenVisitVisitor()
>> -schema.visit(gen)
>> -fdef.write(gen.defn)
>> -fdecl.write(gen.decl)
>> +vis = QAPISchemaGenVisitVisitor()
>> +schema.visit(vis)
>> +genc.body(vis.defn)
>> +genh.body(vis.decl)
>>
>> -close_output(fdef, fdecl)
>> +if do_c:
>> +    genc.write(output_dir, prefix + 'qapi-visit.c')
>> +if do_h:
>> +    genh.write(output_dir, prefix + 'qapi-visit.h')
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index d0816f7479..d73ef618e2 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -2,7 +2,7 @@
>>  # QAPI helper library
>>  #
>>  # Copyright IBM, Corp. 2011
>> -# Copyright (c) 2013-2016 Red Hat Inc.
>> +# Copyright (c) 2013-2018 Red Hat Inc.
>>  #
>>  # Authors:
>>  #  Anthony Liguori <aliguori@us.ibm.com>
>> @@ -1820,7 +1820,6 @@ def guardname(filename):
>>
>>  def guardstart(name):
>>      return mcgen('''
>> -
>>  #ifndef %(name)s
>>  #define %(name)s
>>
>> @@ -1832,7 +1831,6 @@ def guardend(name):
>>      return mcgen('''
>>
>>  #endif /* %(name)s */
>> -
>>  ''',
>>                   name=guardname(name))
>>
>> @@ -1970,17 +1968,53 @@ def parse_command_line(extra_options='', extra_long_options=[]):
>>
>>      return (fname, output_dir, do_c, do_h, prefix, extra_opts)
>>
>> +
>>  #
>> -# Generate output files with boilerplate
>> +# Accumulate and write output
>>  #
>>
>> +class QAPIGen(object):
>> +
>> +    def __init__(self):
>> +        self._preamble = ''
>> +        self._body = ''
>> +
>> +    def preamble(self, text):
>> +        self._preamble += text
>> +
>> +    def body(self, text):
>> +        self._body += text
>> +
>> +    def top(self, fname):
>> +        return ''
>> +
>> +    def bottom(self, fname):
>> +        return ''
>> +
>
> Some methods appends, other return. That's a bit confusing. Why not
> name them accordingly? add_preamble, add_body, get_top...?

You're right, the functions called for side effects have less than
obvious names.  I think I'll keep the noun names for the pure functions.

>> +    def write(self, output_dir, fname):
>> +        if output_dir:
>> +            try:
>> +                os.makedirs(output_dir)
>> +            except os.error as e:
>> +                if e.errno != errno.EEXIST:
>> +                    raise
>> +        f = open(os.path.join(output_dir, fname), 'w')
>> +        f.write(self.top(fname) + self._preamble + self._body
>> +                + self.bottom(fname))
>> +        f.close()
>> +
>> +
>> +class QAPIGenC(QAPIGen):
>> +
>> +    def __init__(self, blurb, pydoc):
>> +        QAPIGen.__init__(self)
>> +        self._blurb = blurb.strip('\n')
>> +        self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
>> +                                                  re.MULTILINE))
>>
>> -def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
>> -    guard = guardname(prefix + h_file)
>> -    c_file = output_dir + prefix + c_file
>> -    h_file = output_dir + prefix + h_file
>> -    copyright = '\n * '.join(re.findall(r'^Copyright .*', doc, re.MULTILINE))
>> -    comment = mcgen('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> +    def top(self, fname):
>> +        return mcgen('''
>> +/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
>>
>>  /*
>>  %(blurb)s
>> @@ -1992,41 +2026,19 @@ def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
>>   */
>>
>>  ''',
>> -                    blurb=blurb.strip('\n'), copyright=copyright)
>> +                     blurb=self._blurb, copyright=self._copyright)
>>
>> -    if output_dir:
>> -        try:
>> -            os.makedirs(output_dir)
>> -        except os.error as e:
>> -            if e.errno != errno.EEXIST:
>> -                raise
>>
>> -    def maybe_open(really, name, opt):
>> -        if really:
>> -            return open(name, opt)
>> -        else:
>> -            import StringIO
>> -            return StringIO.StringIO()
>> +class QAPIGenH(QAPIGenC):
>>
>> -    fdef = maybe_open(do_c, c_file, 'w')
>> -    fdecl = maybe_open(do_h, h_file, 'w')
>> +    def top(self, fname):
>> +        return QAPIGenC.top(self, fname) + guardstart(fname)
>>
>> -    fdef.write(comment)
>> -    fdecl.write(comment)
>> -    fdecl.write(mcgen('''
>> -#ifndef %(guard)s
>> -#define %(guard)s
>> +    def bottom(self, fname):
>> +        return guardend(fname)
>>
>> -''',
>> -                      guard=guard))
>>
>> -    return (fdef, fdecl)
>> -
>> -
>> -def close_output(fdef, fdecl):
>> -    fdecl.write(mcgen('''
>> -
>> -#endif
>> -'''))
>> -    fdecl.close()
>> -    fdef.close()
>> +class QAPIGenDoc(QAPIGen):
>> +    def top(self, fname):
>> +        return (QAPIGen.top(self, fname)
>> +                + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')
>> --
>> 2.13.6
>>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!
Eric Blake Feb. 5, 2018, 3:46 p.m. UTC | #5
On 02/03/2018 02:49 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 02/02/2018 07:03 AM, Markus Armbruster wrote:
>>> These classes encapsulate accumulating and writing output.
>>>
>>> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
>>> rather shallow: most of the output accumulation is not converted.
>>> Left for later.
>>>
>>> The indentation machinery uses a single global variable indent_level,
>>> even though we generally interleave creation of a .c and its .h.  It
>>> should become instance variable of QAPIGenC.  Also left for later.
>>>
>>> Documentation generation isn't converted, and QAPIGenDoc isn't used.
>>> This will change shortly.
>>>

>>>  schema = QAPISchema(input_file)
>>> -gen = QAPISchemaGenEventVisitor()
>>> -schema.visit(gen)
>>> -fdef.write(gen.defn)
>>> -fdecl.write(gen.decl)
>>> +vis = QAPISchemaGenEventVisitor()
>>> +schema.visit(vis)
>>> +genc.body(vis.defn)
>>> +genh.body(vis.decl)
>>
>> I don't know if it is worth a sentence in the commit message that the
>> visitor variable is renamed from 'gen' to 'vis' for less confusion with
>> the new class instances 'genc' and 'genh'.
> 
> Did the rename give you pause when reviewing?

Enough to question whether it was intentional, since it wasn't mentioned
in the commit message (I obviously figured out that it was intentional
and useful, but the fact that I even pointed it out meant that I did
pause during the review).
Markus Armbruster Feb. 6, 2018, 7:28 a.m. UTC | #6
Eric Blake <eblake@redhat.com> writes:

> On 02/03/2018 02:49 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 02/02/2018 07:03 AM, Markus Armbruster wrote:
>>>> These classes encapsulate accumulating and writing output.
>>>>
>>>> Convert C code generation to QAPIGenC and QAPIGenH.  The conversion is
>>>> rather shallow: most of the output accumulation is not converted.
>>>> Left for later.
>>>>
>>>> The indentation machinery uses a single global variable indent_level,
>>>> even though we generally interleave creation of a .c and its .h.  It
>>>> should become instance variable of QAPIGenC.  Also left for later.
>>>>
>>>> Documentation generation isn't converted, and QAPIGenDoc isn't used.
>>>> This will change shortly.
>>>>
>
>>>>  schema = QAPISchema(input_file)
>>>> -gen = QAPISchemaGenEventVisitor()
>>>> -schema.visit(gen)
>>>> -fdef.write(gen.defn)
>>>> -fdecl.write(gen.decl)
>>>> +vis = QAPISchemaGenEventVisitor()
>>>> +schema.visit(vis)
>>>> +genc.body(vis.defn)
>>>> +genh.body(vis.decl)
>>>
>>> I don't know if it is worth a sentence in the commit message that the
>>> visitor variable is renamed from 'gen' to 'vis' for less confusion with
>>> the new class instances 'genc' and 'genh'.
>> 
>> Did the rename give you pause when reviewing?
>
> Enough to question whether it was intentional, since it wasn't mentioned
> in the commit message (I obviously figured out that it was intentional
> and useful, but the fact that I even pointed it out meant that I did
> pause during the review).

Okay, I'll try to make it clearer.
diff mbox series

Patch

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index a861ac52e7..4be7dbc482 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -260,12 +260,10 @@  blurb = '''
  * Schema-defined QAPI/QMP commands
 '''
 
-(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
-                            'qmp-marshal.c', 'qmp-commands.h',
-                            blurb, __doc__)
-
-fdef.write(mcgen('''
+genc = QAPIGenC(blurb, __doc__)
+genh = QAPIGenH(blurb, __doc__)
 
+genc.body(mcgen('''
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/module.h"
@@ -279,21 +277,24 @@  fdef.write(mcgen('''
 #include "%(prefix)sqmp-commands.h"
 
 ''',
-                 prefix=prefix))
+                prefix=prefix))
 
-fdecl.write(mcgen('''
+genh.body(mcgen('''
 #include "%(prefix)sqapi-types.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/dispatch.h"
 
 void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 ''',
-                  prefix=prefix, c_prefix=c_name(prefix, protect=False)))
+                prefix=prefix, c_prefix=c_name(prefix, protect=False)))
 
 schema = QAPISchema(input_file)
-gen = QAPISchemaGenCommandVisitor()
-schema.visit(gen)
-fdef.write(gen.defn)
-fdecl.write(gen.decl)
+vis = QAPISchemaGenCommandVisitor()
+schema.visit(vis)
+genc.body(vis.defn)
+genh.body(vis.decl)
 
-close_output(fdef, fdecl)
+if do_c:
+    genc.write(output_dir, prefix + 'qmp-marshal.c')
+if do_h:
+    genh.write(output_dir, prefix + 'qmp-commands.h')
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index b1d611c5ea..da3de17c76 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -176,11 +176,10 @@  blurb = '''
  * Schema-defined QAPI/QMP events
 '''
 
-(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
-                            'qapi-event.c', 'qapi-event.h',
-                            blurb, __doc__)
+genc = QAPIGenC(blurb, __doc__)
+genh = QAPIGenH(blurb, __doc__)
 
-fdef.write(mcgen('''
+genc.body(mcgen('''
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "%(prefix)sqapi-event.h"
@@ -190,22 +189,25 @@  fdef.write(mcgen('''
 #include "qapi/qmp-event.h"
 
 ''',
-                 prefix=prefix))
+                prefix=prefix))
 
-fdecl.write(mcgen('''
+genh.body(mcgen('''
 #include "qapi/util.h"
 #include "qapi/qmp/qdict.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
-                  prefix=prefix))
+                prefix=prefix))
 
 event_enum_name = c_name(prefix + 'QAPIEvent', protect=False)
 
 schema = QAPISchema(input_file)
-gen = QAPISchemaGenEventVisitor()
-schema.visit(gen)
-fdef.write(gen.defn)
-fdecl.write(gen.decl)
+vis = QAPISchemaGenEventVisitor()
+schema.visit(vis)
+genc.body(vis.defn)
+genh.body(vis.decl)
 
-close_output(fdef, fdecl)
+if do_c:
+    genc.write(output_dir, prefix + 'qapi-event.c')
+if do_h:
+    genh.write(output_dir, prefix + 'qapi-event.h')
diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
index bd9253a172..c654f8fa94 100644
--- a/scripts/qapi-introspect.py
+++ b/scripts/qapi-introspect.py
@@ -181,21 +181,23 @@  blurb = '''
  * QAPI/QMP schema introspection
 '''
 
-(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
-                            'qmp-introspect.c', 'qmp-introspect.h',
-                            blurb, __doc__)
+genc = QAPIGenC(blurb, __doc__)
+genh = QAPIGenH(blurb, __doc__)
 
-fdef.write(mcgen('''
+genc.body(mcgen('''
 #include "qemu/osdep.h"
 #include "%(prefix)sqmp-introspect.h"
 
 ''',
-                 prefix=prefix))
+                prefix=prefix))
 
 schema = QAPISchema(input_file)
-gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
-schema.visit(gen)
-fdef.write(gen.defn)
-fdecl.write(gen.decl)
+vis = QAPISchemaGenIntrospectVisitor(opt_unmask)
+schema.visit(vis)
+genc.body(vis.defn)
+genh.body(vis.decl)
 
-close_output(fdef, fdecl)
+if do_c:
+    genc.write(output_dir, prefix + 'qmp-introspect.c')
+if do_h:
+    genh.write(output_dir, prefix + 'qmp-introspect.h')
diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 1103dbda2d..97406b3368 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -180,7 +180,7 @@  class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
         self.decl = ''
         self.defn = ''
         self._fwdecl = ''
-        self._btin = guardstart('QAPI_TYPES_BUILTIN')
+        self._btin = '\n' + guardstart('QAPI_TYPES_BUILTIN')
 
     def visit_end(self):
         self.decl = self._fwdecl + self.decl
@@ -256,26 +256,28 @@  blurb = '''
  * Schema-defined QAPI types
 '''
 
-(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
-                            'qapi-types.c', 'qapi-types.h',
-                            blurb, __doc__)
+genc = QAPIGenC(blurb, __doc__)
+genh = QAPIGenH(blurb, __doc__)
 
-fdef.write(mcgen('''
+genc.body(mcgen('''
 #include "qemu/osdep.h"
 #include "qapi/dealloc-visitor.h"
 #include "%(prefix)sqapi-types.h"
 #include "%(prefix)sqapi-visit.h"
 ''',
-                 prefix=prefix))
+                prefix=prefix))
 
-fdecl.write(mcgen('''
+genh.body(mcgen('''
 #include "qapi/util.h"
 '''))
 
 schema = QAPISchema(input_file)
-gen = QAPISchemaGenTypeVisitor()
-schema.visit(gen)
-fdef.write(gen.defn)
-fdecl.write(gen.decl)
+vis = QAPISchemaGenTypeVisitor()
+schema.visit(vis)
+genc.body(vis.defn)
+genh.body(vis.decl)
 
-close_output(fdef, fdecl)
+if do_c:
+    genc.write(output_dir, prefix + 'qapi-types.c')
+if do_h:
+    genh.write(output_dir, prefix + 'qapi-types.h')
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 5231f89c36..d1b25daf0d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -339,30 +339,32 @@  blurb = '''
  * Schema-defined QAPI visitors
 '''
 
-(fdef, fdecl) = open_output(output_dir, do_c, do_h, prefix,
-                            'qapi-visit.c', 'qapi-visit.h',
-                            blurb, __doc__)
+genc = QAPIGenC(blurb, __doc__)
+genh = QAPIGenH(blurb, __doc__)
 
-fdef.write(mcgen('''
+genc.body(mcgen('''
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qapi/error.h"
 #include "%(prefix)sqapi-visit.h"
 ''',
-                 prefix=prefix))
+                prefix=prefix))
 
-fdecl.write(mcgen('''
+genh.body(mcgen('''
 #include "qapi/visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "%(prefix)sqapi-types.h"
 
 ''',
-                  prefix=prefix))
+                prefix=prefix))
 
 schema = QAPISchema(input_file)
-gen = QAPISchemaGenVisitVisitor()
-schema.visit(gen)
-fdef.write(gen.defn)
-fdecl.write(gen.decl)
+vis = QAPISchemaGenVisitVisitor()
+schema.visit(vis)
+genc.body(vis.defn)
+genh.body(vis.decl)
 
-close_output(fdef, fdecl)
+if do_c:
+    genc.write(output_dir, prefix + 'qapi-visit.c')
+if do_h:
+    genh.write(output_dir, prefix + 'qapi-visit.h')
diff --git a/scripts/qapi.py b/scripts/qapi.py
index d0816f7479..d73ef618e2 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -2,7 +2,7 @@ 
 # QAPI helper library
 #
 # Copyright IBM, Corp. 2011
-# Copyright (c) 2013-2016 Red Hat Inc.
+# Copyright (c) 2013-2018 Red Hat Inc.
 #
 # Authors:
 #  Anthony Liguori <aliguori@us.ibm.com>
@@ -1820,7 +1820,6 @@  def guardname(filename):
 
 def guardstart(name):
     return mcgen('''
-
 #ifndef %(name)s
 #define %(name)s
 
@@ -1832,7 +1831,6 @@  def guardend(name):
     return mcgen('''
 
 #endif /* %(name)s */
-
 ''',
                  name=guardname(name))
 
@@ -1970,17 +1968,53 @@  def parse_command_line(extra_options='', extra_long_options=[]):
 
     return (fname, output_dir, do_c, do_h, prefix, extra_opts)
 
+
 #
-# Generate output files with boilerplate
+# Accumulate and write output
 #
 
+class QAPIGen(object):
+
+    def __init__(self):
+        self._preamble = ''
+        self._body = ''
+
+    def preamble(self, text):
+        self._preamble += text
+
+    def body(self, text):
+        self._body += text
+
+    def top(self, fname):
+        return ''
+
+    def bottom(self, fname):
+        return ''
+
+    def write(self, output_dir, fname):
+        if output_dir:
+            try:
+                os.makedirs(output_dir)
+            except os.error as e:
+                if e.errno != errno.EEXIST:
+                    raise
+        f = open(os.path.join(output_dir, fname), 'w')
+        f.write(self.top(fname) + self._preamble + self._body
+                + self.bottom(fname))
+        f.close()
+
+
+class QAPIGenC(QAPIGen):
+
+    def __init__(self, blurb, pydoc):
+        QAPIGen.__init__(self)
+        self._blurb = blurb.strip('\n')
+        self._copyright = '\n * '.join(re.findall(r'^Copyright .*', pydoc,
+                                                  re.MULTILINE))
 
-def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
-    guard = guardname(prefix + h_file)
-    c_file = output_dir + prefix + c_file
-    h_file = output_dir + prefix + h_file
-    copyright = '\n * '.join(re.findall(r'^Copyright .*', doc, re.MULTILINE))
-    comment = mcgen('''/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
+    def top(self, fname):
+        return mcgen('''
+/* AUTOMATICALLY GENERATED, DO NOT MODIFY */
 
 /*
 %(blurb)s
@@ -1992,41 +2026,19 @@  def open_output(output_dir, do_c, do_h, prefix, c_file, h_file, blurb, doc):
  */
 
 ''',
-                    blurb=blurb.strip('\n'), copyright=copyright)
+                     blurb=self._blurb, copyright=self._copyright)
 
-    if output_dir:
-        try:
-            os.makedirs(output_dir)
-        except os.error as e:
-            if e.errno != errno.EEXIST:
-                raise
 
-    def maybe_open(really, name, opt):
-        if really:
-            return open(name, opt)
-        else:
-            import StringIO
-            return StringIO.StringIO()
+class QAPIGenH(QAPIGenC):
 
-    fdef = maybe_open(do_c, c_file, 'w')
-    fdecl = maybe_open(do_h, h_file, 'w')
+    def top(self, fname):
+        return QAPIGenC.top(self, fname) + guardstart(fname)
 
-    fdef.write(comment)
-    fdecl.write(comment)
-    fdecl.write(mcgen('''
-#ifndef %(guard)s
-#define %(guard)s
+    def bottom(self, fname):
+        return guardend(fname)
 
-''',
-                      guard=guard))
 
-    return (fdef, fdecl)
-
-
-def close_output(fdef, fdecl):
-    fdecl.write(mcgen('''
-
-#endif
-'''))
-    fdecl.close()
-    fdef.close()
+class QAPIGenDoc(QAPIGen):
+    def top(self, fname):
+        return (QAPIGen.top(self, fname)
+                + '@c AUTOMATICALLY GENERATED, DO NOT MODIFY\n\n')