Patchwork [09/11] qapi: add qapi-errors.py

login
register
mail settings
Submitter Luiz Capitulino
Date July 25, 2012, 4:54 p.m.
Message ID <1343235256-26310-10-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/173229/
State New
Headers show

Comments

Luiz Capitulino - July 25, 2012, 4:54 p.m.
This script generates two files from qapi-schema-errors.json:

 o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
                  corresponds to most of today's qerror.h

 o qapi-errors.c: contains the error table that currently exists in qerror.c

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile               |   8 ++-
 scripts/qapi-errors.py | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 183 insertions(+), 2 deletions(-)
 create mode 100644 scripts/qapi-errors.py
Markus Armbruster - July 26, 2012, 11:50 a.m.
Paolo, Eric, I got a make puzzle for you below.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This script generates two files from qapi-schema-errors.json:
>
>  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
>                   corresponds to most of today's qerror.h
>
>  o qapi-errors.c: contains the error table that currently exists in qerror.c
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  Makefile               |   8 ++-
>  scripts/qapi-errors.py | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 183 insertions(+), 2 deletions(-)
>  create mode 100644 scripts/qapi-errors.py
>
> diff --git a/Makefile b/Makefile
> index ab82ef3..2cdc732 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -22,8 +22,9 @@ GENERATED_HEADERS = config-host.h trace.h qemu-options.def
>  ifeq ($(TRACE_BACKEND),dtrace)
>  GENERATED_HEADERS += trace-dtrace.h
>  endif
> -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
> +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-errors.h
> +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-errors.c \
> +                     trace.c
>  
>  # Don't try to regenerate Makefile or configure
>  # We don't generate any of them
> @@ -200,6 +201,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
>  qmp-commands.h qmp-marshal.c :\
>  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
>  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> +qapi-errors.h qapi-errors.c :\
> +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")

I'm afraid this isn't quite what you want.  It's shorthand for two
separate rules with the same recipe[*].  Therefore, it's prone to run
the recipe twice, with make blissfully unaware that each of the two runs
clobbers the other file, too.  Could conceivably lead to trouble with
parallel execution.

Paolo, Eric, maybe you can provide advice on how to best tell make that
a recipe generates multiple files.

>  QGALIB_OBJ=$(addprefix qapi-generated/, qga-qapi-types.o qga-qapi-visit.o qga-qmp-marshal.o)
>  QGALIB_GEN=$(addprefix qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
> diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
> new file mode 100644
> index 0000000..59cf426
> --- /dev/null
> +++ b/scripts/qapi-errors.py
> @@ -0,0 +1,177 @@
> +#
> +# QAPI errors generator
> +#
> +# Copyright (C) 2012 Red Hat, Inc.
> +#
> +# This work is licensed under the terms of the GNU GPLv2.

Sure you want v2 and not v2+?

> +# See the COPYING.LIB file in the top-level directory.

COPYING.LIB is for LGPL, use COPYING with GPL.

> +from ordereddict import OrderedDict
> +import getopt, sys, os, errno
> +from qapi import *
> +
> +def gen_error_decl_prologue(header, guard, prefix=""):
> +    ret = mcgen('''
> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * schema-defined QAPI Errors
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef %(guard)s
> +#define %(guard)s
> +
> +''',
> +                 header=basename(header), guard=guardname(header), prefix=prefix)
> +    return ret
> +
> +def gen_error_def_prologue(error_header, prefix=""):
> +    ret = mcgen('''
> +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> +
> +/*
> + * schema-defined QMP Error table
> + *
> + * Copyright (C) 2012 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "%(prefix)s%(error_header)s"
> +
> +''',
> +                prefix=prefix, error_header=error_header)
> +    return ret
> +
> +def gen_error_macro(err_domain):
> +    string = ''
> +    cur = err_domain[0]
> +    for nxt in err_domain[1:]:

"err_domain" is just a fancy name for the error symbol, i.e. what
qerror.h calls 'class', isn't it?

Is it the same as error_class in gen_error_decl_macros() below?

> +        if string and cur.isupper() and nxt.islower():
> +            string += '_'
> +        string += cur
> +        cur = nxt
> +    string += cur
> +    return 'QERR_' + string.upper()
> +
> +def gen_error_def_table(exprs):
> +    ret = mcgen('''
> +static const QErrorStringTable qerror_table[] = {
> +''')
> +
> +    for err in exprs:
> +        macro = gen_error_macro(err['error'])
> +        desc = err['description']
> +        ret += mcgen('''
> +        {
> +            .error_fmt = %(error_macro)s,
> +            .desc      = "%(error_desc)s",
> +        },
> +''',    
> +                    error_macro=macro, error_desc=desc)
> +
> +    ret += mcgen('''
> +    {}
> +};
> +''')
> +
> +    return ret
> +
> +def gen_error_macro_data_str(data):
> +    colon = ''
> +    data_str = ''
> +    for k, v in data.items():
> +        data_str += colon + "'%s': " % k
> +        if v == 'str':
> +            data_str += "%s"
> +        elif v == 'int':
> +            data_str += '%"PRId64"'

PRId64 matches existing qerror.h practice.  Requires the macro's users
to pass suitable argument, which can be bothersome, but at least the
compiler helps with it.

> +        else:
> +            sys.exit("unknown data type '%s' for error '%s'" % (v, name))
> +        colon = ', '

colon is either empty or ', ', but never a colon.  What about calling it
sep, for separator?

> +    return data_str
> +
> +def gen_error_decl_macros(exprs):
> +    ret = ''
> +    for err in exprs:
> +        data = gen_error_macro_data_str({})
> +        if err.has_key('data'):
> +            data = gen_error_macro_data_str(err['data'])

Wouldn't 
           if err.has_key('data'):
               data = gen_error_macro_data_str(err['data'])
           else:
               data = gen_error_macro_data_str({})

be clearer?

> +        macro = gen_error_macro(err['error'])
> +        name = err['error']
> +
> +        ret += mcgen('''
> +#define %(error_macro)s \\
> +    "{ 'class': '%(error_class)s', 'data': { %(error_data)s } }"
> +
> +''',
> +                error_macro=macro, error_class=name, error_data=data)
> +    return ret
> +
> +def maybe_open(really, name, opt):
> +    if really:
> +        return open(name, opt)
> +    else:
> +        import StringIO
> +        return StringIO.StringIO()
> +
> +if __name__ == '__main__':
> +    try:
> +        opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> +                                       ["prefix=", "output-dir="])
> +    except getopt.GetoptError, err:
> +        print str(err)
> +        sys.exit(1)
> +
> +    prefix = ""
> +    output_dir = ""
> +    do_c = True
> +    do_h = True

Both are never false.  Purpose?

> +    c_file = 'qapi-errors.c'
> +    h_file = 'qapi-errors.h'
> +
> +    for o, a in opts:
> +        if o in ("-p", "--prefix"):
> +            prefix = a
> +        elif o in ("-o", "--output-dir"):
> +            output_dir = a + "/"
> +
> +    c_file = output_dir + prefix + c_file
> +    h_file = output_dir + prefix + h_file
> +
> +    try:
> +        os.makedirs(output_dir)
> +    except os.error, e:
> +        if e.errno != errno.EEXIST:
> +            raise
> +
> +    exprs = parse_schema(sys.stdin)
> +
> +    fdecl = maybe_open(do_h, h_file, 'w')
> +    ret = gen_error_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
> +    fdecl.write(ret)
> +
> +    ret = gen_error_decl_macros(exprs)
> +    fdecl.write(ret)
> +
> +    fdecl.write("#endif\n")
> +    fdecl.flush()
> +    fdecl.close()

Err, is flush before close really necessary?

> +
> +    fdef = maybe_open(do_c, c_file, 'w')
> +    ret = gen_error_def_prologue(error_header=h_file, prefix=prefix)
> +    fdef.write(ret)
> +
> +    ret = gen_error_def_table(exprs)
> +    fdef.write(ret)
> +
> +    fdef.flush()
> +    fdef.close()


[*] http://www.gnu.org/software/make/manual/make.html#Multiple-Targets
Paolo Bonzini - July 26, 2012, 11:55 a.m.
Il 26/07/2012 13:50, Markus Armbruster ha scritto:
>> > +qapi-errors.h qapi-errors.c :\
>> > +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
>> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
> I'm afraid this isn't quite what you want.  It's shorthand for two
> separate rules with the same recipe[*].  Therefore, it's prone to run
> the recipe twice, with make blissfully unaware that each of the two runs
> clobbers the other file, too.  Could conceivably lead to trouble with
> parallel execution.
> 
> Paolo, Eric, maybe you can provide advice on how to best tell make that
> a recipe generates multiple files.

Hmm, I would just do

qapi-errors.h: qapi-errors.c
qapi-errors.c: $(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")

I think that's what I usually saw for bison (which creates both .h and .c).

A perhaps cleaner alternative is to add a stamp file, and make both files
depend on it.

Paolo
Markus Armbruster - July 26, 2012, 12:12 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> This script generates two files from qapi-schema-errors.json:
>
>  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
>                   corresponds to most of today's qerror.h
>
>  o qapi-errors.c: contains the error table that currently exists in qerror.c
[...]
> diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
> new file mode 100644
> index 0000000..59cf426
> --- /dev/null
> +++ b/scripts/qapi-errors.py
[...]
> +def gen_error_def_table(exprs):
> +    ret = mcgen('''
> +static const QErrorStringTable qerror_table[] = {
> +''')
> +
> +    for err in exprs:
> +        macro = gen_error_macro(err['error'])
> +        desc = err['description']
> +        ret += mcgen('''
> +        {
> +            .error_fmt = %(error_macro)s,
> +            .desc      = "%(error_desc)s",
> +        },
> +''',    

Trailing whitespace.

[...]
Eric Blake - July 26, 2012, 12:38 p.m.
On 07/26/2012 05:55 AM, Paolo Bonzini wrote:
> Il 26/07/2012 13:50, Markus Armbruster ha scritto:
>>>> +qapi-errors.h qapi-errors.c :\
>>>> +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
>>>> +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
>> I'm afraid this isn't quite what you want.  It's shorthand for two
>> separate rules with the same recipe[*].  Therefore, it's prone to run
>> the recipe twice, with make blissfully unaware that each of the two runs
>> clobbers the other file, too.  Could conceivably lead to trouble with
>> parallel execution.
>>
>> Paolo, Eric, maybe you can provide advice on how to best tell make that
>> a recipe generates multiple files.
> 
> Hmm, I would just do
> 
> qapi-errors.h: qapi-errors.c
> qapi-errors.c: $(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
> 
> I think that's what I usually saw for bison (which creates both .h and .c).

Indeed, per
https://www.gnu.org/software/automake/manual/automake.html#Multiple-Outputs,
that is an appropriate solution for a 2-file generation.  It is only
when you have more than two files where...

> 
> A perhaps cleaner alternative is to add a stamp file, and make both files
> depend on it.

a stamp file makes more sense.
Eric Blake - July 26, 2012, 12:42 p.m.
On 07/26/2012 06:38 AM, Eric Blake wrote:
>>> Paolo, Eric, maybe you can provide advice on how to best tell make that
>>> a recipe generates multiple files.
>>
>> Hmm, I would just do
>>
>> qapi-errors.h: qapi-errors.c
>> qapi-errors.c: $(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
>> 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
>>
>> I think that's what I usually saw for bison (which creates both .h and .c).
> 
> Indeed, per
> https://www.gnu.org/software/automake/manual/automake.html#Multiple-Outputs,
> that is an appropriate solution for a 2-file generation.

Or, since we depend on GNU make, we can use pattern rules instead
(untested):

%-errors.h %-errors.c: \
   %(SRC_PATH)/%-schema-errors.json $(SRC_PATH)/srcipts/%-errors.py
	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o
"." < $<, "  GEN   $@")

https://www.gnu.org/software/automake/manual/make.html#Pattern-Examples
Luiz Capitulino - July 26, 2012, 2:34 p.m.
On Thu, 26 Jul 2012 13:50:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Paolo, Eric, I got a make puzzle for you below.
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This script generates two files from qapi-schema-errors.json:
> >
> >  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
> >                   corresponds to most of today's qerror.h
> >
> >  o qapi-errors.c: contains the error table that currently exists in qerror.c
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  Makefile               |   8 ++-
> >  scripts/qapi-errors.py | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 183 insertions(+), 2 deletions(-)
> >  create mode 100644 scripts/qapi-errors.py
> >
> > diff --git a/Makefile b/Makefile
> > index ab82ef3..2cdc732 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -22,8 +22,9 @@ GENERATED_HEADERS = config-host.h trace.h qemu-options.def
> >  ifeq ($(TRACE_BACKEND),dtrace)
> >  GENERATED_HEADERS += trace-dtrace.h
> >  endif
> > -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
> > -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
> > +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-errors.h
> > +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-errors.c \
> > +                     trace.c
> >  
> >  # Don't try to regenerate Makefile or configure
> >  # We don't generate any of them
> > @@ -200,6 +201,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
> >  qmp-commands.h qmp-marshal.c :\
> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
> >  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
> > +qapi-errors.h qapi-errors.c :\
> > +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
> 
> I'm afraid this isn't quite what you want.  It's shorthand for two
> separate rules with the same recipe[*].  Therefore, it's prone to run
> the recipe twice, with make blissfully unaware that each of the two runs
> clobbers the other file, too.  Could conceivably lead to trouble with
> parallel execution.
> 
> Paolo, Eric, maybe you can provide advice on how to best tell make that
> a recipe generates multiple files.
> 
> >  QGALIB_OBJ=$(addprefix qapi-generated/, qga-qapi-types.o qga-qapi-visit.o qga-qmp-marshal.o)
> >  QGALIB_GEN=$(addprefix qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
> > diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
> > new file mode 100644
> > index 0000000..59cf426
> > --- /dev/null
> > +++ b/scripts/qapi-errors.py
> > @@ -0,0 +1,177 @@
> > +#
> > +# QAPI errors generator
> > +#
> > +# Copyright (C) 2012 Red Hat, Inc.
> > +#
> > +# This work is licensed under the terms of the GNU GPLv2.
> 
> Sure you want v2 and not v2+?
> 
> > +# See the COPYING.LIB file in the top-level directory.
> 
> COPYING.LIB is for LGPL, use COPYING with GPL.

I started this script as a copy from the others qapi scripts and didn't
notice that, thanks for spotting it (fixed in v3).

> > +from ordereddict import OrderedDict
> > +import getopt, sys, os, errno
> > +from qapi import *
> > +
> > +def gen_error_decl_prologue(header, guard, prefix=""):
> > +    ret = mcgen('''
> > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > +
> > +/*
> > + * schema-defined QAPI Errors
> > + *
> > + * Copyright (C) 2012 Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +#ifndef %(guard)s
> > +#define %(guard)s
> > +
> > +''',
> > +                 header=basename(header), guard=guardname(header), prefix=prefix)
> > +    return ret
> > +
> > +def gen_error_def_prologue(error_header, prefix=""):
> > +    ret = mcgen('''
> > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
> > +
> > +/*
> > + * schema-defined QMP Error table
> > + *
> > + * Copyright (C) 2012 Red Hat, Inc.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + *
> > + */
> > +
> > +#include "%(prefix)s%(error_header)s"
> > +
> > +''',
> > +                prefix=prefix, error_header=error_header)
> > +    return ret
> > +
> > +def gen_error_macro(err_domain):
> > +    string = ''
> > +    cur = err_domain[0]
> > +    for nxt in err_domain[1:]:
> 
> "err_domain" is just a fancy name for the error symbol, i.e. what
> qerror.h calls 'class', isn't it?
> 
> Is it the same as error_class in gen_error_decl_macros() below?

Do you want to suggest a better name or are you just pointing out the lack of
consistence?

> 
> > +        if string and cur.isupper() and nxt.islower():
> > +            string += '_'
> > +        string += cur
> > +        cur = nxt
> > +    string += cur
> > +    return 'QERR_' + string.upper()
> > +
> > +def gen_error_def_table(exprs):
> > +    ret = mcgen('''
> > +static const QErrorStringTable qerror_table[] = {
> > +''')
> > +
> > +    for err in exprs:
> > +        macro = gen_error_macro(err['error'])
> > +        desc = err['description']
> > +        ret += mcgen('''
> > +        {
> > +            .error_fmt = %(error_macro)s,
> > +            .desc      = "%(error_desc)s",
> > +        },
> > +''',    
> > +                    error_macro=macro, error_desc=desc)
> > +
> > +    ret += mcgen('''
> > +    {}
> > +};
> > +''')
> > +
> > +    return ret
> > +
> > +def gen_error_macro_data_str(data):
> > +    colon = ''
> > +    data_str = ''
> > +    for k, v in data.items():
> > +        data_str += colon + "'%s': " % k
> > +        if v == 'str':
> > +            data_str += "%s"
> > +        elif v == 'int':
> > +            data_str += '%"PRId64"'
> 
> PRId64 matches existing qerror.h practice.  Requires the macro's users
> to pass suitable argument, which can be bothersome, but at least the
> compiler helps with it.
> 
> > +        else:
> > +            sys.exit("unknown data type '%s' for error '%s'" % (v, name))
> > +        colon = ', '
> 
> colon is either empty or ', ', but never a colon.  What about calling it
> sep, for separator?

Done.

> 
> > +    return data_str
> > +
> > +def gen_error_decl_macros(exprs):
> > +    ret = ''
> > +    for err in exprs:
> > +        data = gen_error_macro_data_str({})
> > +        if err.has_key('data'):
> > +            data = gen_error_macro_data_str(err['data'])
> 
> Wouldn't 
>            if err.has_key('data'):
>                data = gen_error_macro_data_str(err['data'])
>            else:
>                data = gen_error_macro_data_str({})
> 
> be clearer?

Makes no difference for me, but I've changed to your suggestion.

> 
> > +        macro = gen_error_macro(err['error'])
> > +        name = err['error']
> > +
> > +        ret += mcgen('''
> > +#define %(error_macro)s \\
> > +    "{ 'class': '%(error_class)s', 'data': { %(error_data)s } }"
> > +
> > +''',
> > +                error_macro=macro, error_class=name, error_data=data)
> > +    return ret
> > +
> > +def maybe_open(really, name, opt):
> > +    if really:
> > +        return open(name, opt)
> > +    else:
> > +        import StringIO
> > +        return StringIO.StringIO()
> > +
> > +if __name__ == '__main__':
> > +    try:
> > +        opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
> > +                                       ["prefix=", "output-dir="])
> > +    except getopt.GetoptError, err:
> > +        print str(err)
> > +        sys.exit(1)
> > +
> > +    prefix = ""
> > +    output_dir = ""
> > +    do_c = True
> > +    do_h = True
> 
> Both are never false.  Purpose?

This was also copied from others qapi scripts, but qapi-errors.py always
creates both files so I've dropped this.

> 
> > +    c_file = 'qapi-errors.c'
> > +    h_file = 'qapi-errors.h'
> > +
> > +    for o, a in opts:
> > +        if o in ("-p", "--prefix"):
> > +            prefix = a
> > +        elif o in ("-o", "--output-dir"):
> > +            output_dir = a + "/"
> > +
> > +    c_file = output_dir + prefix + c_file
> > +    h_file = output_dir + prefix + h_file
> > +
> > +    try:
> > +        os.makedirs(output_dir)
> > +    except os.error, e:
> > +        if e.errno != errno.EEXIST:
> > +            raise
> > +
> > +    exprs = parse_schema(sys.stdin)
> > +
> > +    fdecl = maybe_open(do_h, h_file, 'w')
> > +    ret = gen_error_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
> > +    fdecl.write(ret)
> > +
> > +    ret = gen_error_decl_macros(exprs)
> > +    fdecl.write(ret)
> > +
> > +    fdecl.write("#endif\n")
> > +    fdecl.flush()
> > +    fdecl.close()
> 
> Err, is flush before close really necessary?

I don't think so, but doesn't cause any harm either (and I'll maintain it as
the others qapi scripts do this too).

> 
> > +
> > +    fdef = maybe_open(do_c, c_file, 'w')
> > +    ret = gen_error_def_prologue(error_header=h_file, prefix=prefix)
> > +    fdef.write(ret)
> > +
> > +    ret = gen_error_def_table(exprs)
> > +    fdef.write(ret)
> > +
> > +    fdef.flush()
> > +    fdef.close()
> 
> 
> [*] http://www.gnu.org/software/make/manual/make.html#Multiple-Targets
>
Luiz Capitulino - July 26, 2012, 2:45 p.m.
On Thu, 26 Jul 2012 13:55:29 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 26/07/2012 13:50, Markus Armbruster ha scritto:
> >> > +qapi-errors.h qapi-errors.c :\
> >> > +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> >> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
> > I'm afraid this isn't quite what you want.  It's shorthand for two
> > separate rules with the same recipe[*].  Therefore, it's prone to run
> > the recipe twice, with make blissfully unaware that each of the two runs
> > clobbers the other file, too.  Could conceivably lead to trouble with
> > parallel execution.
> > 
> > Paolo, Eric, maybe you can provide advice on how to best tell make that
> > a recipe generates multiple files.
> 
> Hmm, I would just do
> 
> qapi-errors.h: qapi-errors.c
> qapi-errors.c: $(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
> 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")

I've done this for v3, thanks for the suggestion.

> 
> I think that's what I usually saw for bison (which creates both .h and .c).
> 
> A perhaps cleaner alternative is to add a stamp file, and make both files
> depend on it.
> 
> Paolo
>
Luiz Capitulino - July 26, 2012, 2:47 p.m.
On Thu, 26 Jul 2012 14:12:50 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This script generates two files from qapi-schema-errors.json:
> >
> >  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
> >                   corresponds to most of today's qerror.h
> >
> >  o qapi-errors.c: contains the error table that currently exists in qerror.c
> [...]
> > diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
> > new file mode 100644
> > index 0000000..59cf426
> > --- /dev/null
> > +++ b/scripts/qapi-errors.py
> [...]
> > +def gen_error_def_table(exprs):
> > +    ret = mcgen('''
> > +static const QErrorStringTable qerror_table[] = {
> > +''')
> > +
> > +    for err in exprs:
> > +        macro = gen_error_macro(err['error'])
> > +        desc = err['description']
> > +        ret += mcgen('''
> > +        {
> > +            .error_fmt = %(error_macro)s,
> > +            .desc      = "%(error_desc)s",
> > +        },
> > +''',    
> 
> Trailing whitespace.

Fixed for v3.
Markus Armbruster - July 26, 2012, 4:31 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 26 Jul 2012 13:50:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Paolo, Eric, I got a make puzzle for you below.
>> 
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > This script generates two files from qapi-schema-errors.json:
>> >
>> >  o qapi-errors.h: contains error macro definitions, eg. QERR_BASE_NOT_FOUND,
>> >                   corresponds to most of today's qerror.h
>> >
>> >  o qapi-errors.c: contains the error table that currently exists in qerror.c
>> >
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  Makefile               |   8 ++-
>> >  scripts/qapi-errors.py | 177 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 183 insertions(+), 2 deletions(-)
>> >  create mode 100644 scripts/qapi-errors.py
>> >
>> > diff --git a/Makefile b/Makefile
>> > index ab82ef3..2cdc732 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -22,8 +22,9 @@ GENERATED_HEADERS = config-host.h trace.h qemu-options.def
>> >  ifeq ($(TRACE_BACKEND),dtrace)
>> >  GENERATED_HEADERS += trace-dtrace.h
>> >  endif
>> > -GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
>> > -GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
>> > +GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-errors.h
>> > +GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-errors.c \
>> > +                     trace.c
>> >  
>> >  # Don't try to regenerate Makefile or configure
>> >  # We don't generate any of them
>> > @@ -200,6 +201,9 @@ $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
>> >  qmp-commands.h qmp-marshal.c :\
>> >  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
>> >  	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
>> > +qapi-errors.h qapi-errors.c :\
>> > +$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
>> > +	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
>> 
>> I'm afraid this isn't quite what you want.  It's shorthand for two
>> separate rules with the same recipe[*].  Therefore, it's prone to run
>> the recipe twice, with make blissfully unaware that each of the two runs
>> clobbers the other file, too.  Could conceivably lead to trouble with
>> parallel execution.
>> 
>> Paolo, Eric, maybe you can provide advice on how to best tell make that
>> a recipe generates multiple files.
>> 
>> >  QGALIB_OBJ=$(addprefix qapi-generated/, qga-qapi-types.o
>> > qga-qapi-visit.o qga-qmp-marshal.o)
>> >  QGALIB_GEN=$(addprefix qapi-generated/, qga-qapi-types.h
>> > qga-qapi-visit.h qga-qmp-commands.h)
>> > diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
>> > new file mode 100644
>> > index 0000000..59cf426
>> > --- /dev/null
>> > +++ b/scripts/qapi-errors.py
>> > @@ -0,0 +1,177 @@
>> > +#
>> > +# QAPI errors generator
>> > +#
>> > +# Copyright (C) 2012 Red Hat, Inc.
>> > +#
>> > +# This work is licensed under the terms of the GNU GPLv2.
>> 
>> Sure you want v2 and not v2+?
>> 
>> > +# See the COPYING.LIB file in the top-level directory.
>> 
>> COPYING.LIB is for LGPL, use COPYING with GPL.
>
> I started this script as a copy from the others qapi scripts and didn't
> notice that, thanks for spotting it (fixed in v3).

The other qapi scripts need fixing then --- self-contradictory copyright
statements are not a good idea.

>> > +from ordereddict import OrderedDict
>> > +import getopt, sys, os, errno
>> > +from qapi import *
>> > +
>> > +def gen_error_decl_prologue(header, guard, prefix=""):
>> > +    ret = mcgen('''
>> > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> > +
>> > +/*
>> > + * schema-defined QAPI Errors
>> > + *
>> > + * Copyright (C) 2012 Red Hat, Inc.
>> > + *
>> > + * This work is licensed under the terms of the GNU LGPL, version
>> > 2.1 or later.
>> > + * See the COPYING.LIB file in the top-level directory.
>> > + *
>> > + */
>> > +
>> > +#ifndef %(guard)s
>> > +#define %(guard)s
>> > +
>> > +''',
>> > + header=basename(header), guard=guardname(header), prefix=prefix)
>> > +    return ret
>> > +
>> > +def gen_error_def_prologue(error_header, prefix=""):
>> > +    ret = mcgen('''
>> > +/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
>> > +
>> > +/*
>> > + * schema-defined QMP Error table
>> > + *
>> > + * Copyright (C) 2012 Red Hat, Inc.
>> > + *
>> > + * This work is licensed under the terms of the GNU LGPL, version
>> > 2.1 or later.
>> > + * See the COPYING.LIB file in the top-level directory.
>> > + *
>> > + */
>> > +
>> > +#include "%(prefix)s%(error_header)s"
>> > +
>> > +''',
>> > +                prefix=prefix, error_header=error_header)
>> > +    return ret
>> > +
>> > +def gen_error_macro(err_domain):
>> > +    string = ''
>> > +    cur = err_domain[0]
>> > +    for nxt in err_domain[1:]:
>> 
>> "err_domain" is just a fancy name for the error symbol, i.e. what
>> qerror.h calls 'class', isn't it?
>> 
>> Is it the same as error_class in gen_error_decl_macros() below?
>
> Do you want to suggest a better name or are you just pointing out the lack of
> consistence?

No, I'm just trying to understand what's going on, and asking you to
confirm my hypothesis.

But yes, same name for same thing would be nice.

>> > +        if string and cur.isupper() and nxt.islower():
>> > +            string += '_'
>> > +        string += cur
>> > +        cur = nxt
>> > +    string += cur
>> > +    return 'QERR_' + string.upper()
>> > +
>> > +def gen_error_def_table(exprs):
>> > +    ret = mcgen('''
>> > +static const QErrorStringTable qerror_table[] = {
>> > +''')
>> > +
>> > +    for err in exprs:
>> > +        macro = gen_error_macro(err['error'])
>> > +        desc = err['description']
>> > +        ret += mcgen('''
>> > +        {
>> > +            .error_fmt = %(error_macro)s,
>> > +            .desc      = "%(error_desc)s",
>> > +        },
>> > +''',    
>> > +                    error_macro=macro, error_desc=desc)
>> > +
>> > +    ret += mcgen('''
>> > +    {}
>> > +};
>> > +''')
>> > +
>> > +    return ret
>> > +
>> > +def gen_error_macro_data_str(data):
>> > +    colon = ''
>> > +    data_str = ''
>> > +    for k, v in data.items():
>> > +        data_str += colon + "'%s': " % k
>> > +        if v == 'str':
>> > +            data_str += "%s"
>> > +        elif v == 'int':
>> > +            data_str += '%"PRId64"'
>> 
>> PRId64 matches existing qerror.h practice.  Requires the macro's users
>> to pass suitable argument, which can be bothersome, but at least the
>> compiler helps with it.
>> 
>> > +        else:
>> > +            sys.exit("unknown data type '%s' for error '%s'" % (v, name))
>> > +        colon = ', '
>> 
>> colon is either empty or ', ', but never a colon.  What about calling it
>> sep, for separator?
>
> Done.
>
>> 
>> > +    return data_str
>> > +
>> > +def gen_error_decl_macros(exprs):
>> > +    ret = ''
>> > +    for err in exprs:
>> > +        data = gen_error_macro_data_str({})
>> > +        if err.has_key('data'):
>> > +            data = gen_error_macro_data_str(err['data'])
>> 
>> Wouldn't 
>>            if err.has_key('data'):
>>                data = gen_error_macro_data_str(err['data'])
>>            else:
>>                data = gen_error_macro_data_str({})
>> 
>> be clearer?
>
> Makes no difference for me, but I've changed to your suggestion.
>
>> 
>> > +        macro = gen_error_macro(err['error'])
>> > +        name = err['error']
>> > +
>> > +        ret += mcgen('''
>> > +#define %(error_macro)s \\
>> > +    "{ 'class': '%(error_class)s', 'data': { %(error_data)s } }"
>> > +
>> > +''',
>> > +                error_macro=macro, error_class=name, error_data=data)
>> > +    return ret
>> > +
>> > +def maybe_open(really, name, opt):
>> > +    if really:
>> > +        return open(name, opt)
>> > +    else:
>> > +        import StringIO
>> > +        return StringIO.StringIO()
>> > +
>> > +if __name__ == '__main__':
>> > +    try:
>> > +        opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
>> > +                                       ["prefix=", "output-dir="])
>> > +    except getopt.GetoptError, err:
>> > +        print str(err)
>> > +        sys.exit(1)
>> > +
>> > +    prefix = ""
>> > +    output_dir = ""
>> > +    do_c = True
>> > +    do_h = True
>> 
>> Both are never false.  Purpose?
>
> This was also copied from others qapi scripts, but qapi-errors.py always
> creates both files so I've dropped this.
>
>> 
>> > +    c_file = 'qapi-errors.c'
>> > +    h_file = 'qapi-errors.h'
>> > +
>> > +    for o, a in opts:
>> > +        if o in ("-p", "--prefix"):
>> > +            prefix = a
>> > +        elif o in ("-o", "--output-dir"):
>> > +            output_dir = a + "/"
>> > +
>> > +    c_file = output_dir + prefix + c_file
>> > +    h_file = output_dir + prefix + h_file
>> > +
>> > +    try:
>> > +        os.makedirs(output_dir)
>> > +    except os.error, e:
>> > +        if e.errno != errno.EEXIST:
>> > +            raise
>> > +
>> > +    exprs = parse_schema(sys.stdin)
>> > +
>> > +    fdecl = maybe_open(do_h, h_file, 'w')
>> > + ret = gen_error_decl_prologue(header=basename(h_file),
>> > guard=guardname(h_file), prefix=prefix)
>> > +    fdecl.write(ret)
>> > +
>> > +    ret = gen_error_decl_macros(exprs)
>> > +    fdecl.write(ret)
>> > +
>> > +    fdecl.write("#endif\n")
>> > +    fdecl.flush()
>> > +    fdecl.close()
>> 
>> Err, is flush before close really necessary?
>
> I don't think so, but doesn't cause any harm either (and I'll maintain it as
> the others qapi scripts do this too).

Looks like cargo cult programming to me :)

>> > +
>> > +    fdef = maybe_open(do_c, c_file, 'w')
>> > +    ret = gen_error_def_prologue(error_header=h_file, prefix=prefix)
>> > +    fdef.write(ret)
>> > +
>> > +    ret = gen_error_def_table(exprs)
>> > +    fdef.write(ret)
>> > +
>> > +    fdef.flush()
>> > +    fdef.close()
>> 
>> 
>> [*] http://www.gnu.org/software/make/manual/make.html#Multiple-Targets

Patch

diff --git a/Makefile b/Makefile
index ab82ef3..2cdc732 100644
--- a/Makefile
+++ b/Makefile
@@ -22,8 +22,9 @@  GENERATED_HEADERS = config-host.h trace.h qemu-options.def
 ifeq ($(TRACE_BACKEND),dtrace)
 GENERATED_HEADERS += trace-dtrace.h
 endif
-GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h
-GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c trace.c
+GENERATED_HEADERS += qmp-commands.h qapi-types.h qapi-visit.h qapi-errors.h
+GENERATED_SOURCES += qmp-marshal.c qapi-types.c qapi-visit.c qapi-errors.c \
+                     trace.c
 
 # Don't try to regenerate Makefile or configure
 # We don't generate any of them
@@ -200,6 +201,9 @@  $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-visit.py
 qmp-commands.h qmp-marshal.c :\
 $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py
 	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-commands.py $(gen-out-type) -m -o "." < $<, "  GEN   $@")
+qapi-errors.h qapi-errors.c :\
+$(SRC_PATH)/qapi-schema-errors.json $(SRC_PATH)/scripts/qapi-errors.py
+	$(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-errors.py -o "." < $<, "  GEN   $@")
 
 QGALIB_OBJ=$(addprefix qapi-generated/, qga-qapi-types.o qga-qapi-visit.o qga-qmp-marshal.o)
 QGALIB_GEN=$(addprefix qapi-generated/, qga-qapi-types.h qga-qapi-visit.h qga-qmp-commands.h)
diff --git a/scripts/qapi-errors.py b/scripts/qapi-errors.py
new file mode 100644
index 0000000..59cf426
--- /dev/null
+++ b/scripts/qapi-errors.py
@@ -0,0 +1,177 @@ 
+#
+# QAPI errors generator
+#
+# Copyright (C) 2012 Red Hat, Inc.
+#
+# This work is licensed under the terms of the GNU GPLv2.
+# See the COPYING.LIB file in the top-level directory.
+
+from ordereddict import OrderedDict
+import getopt, sys, os, errno
+from qapi import *
+
+def gen_error_decl_prologue(header, guard, prefix=""):
+    ret = mcgen('''
+/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
+
+/*
+ * schema-defined QAPI Errors
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef %(guard)s
+#define %(guard)s
+
+''',
+                 header=basename(header), guard=guardname(header), prefix=prefix)
+    return ret
+
+def gen_error_def_prologue(error_header, prefix=""):
+    ret = mcgen('''
+/* THIS FILE IS AUTOMATICALLY GENERATED, DO NOT MODIFY */
+
+/*
+ * schema-defined QMP Error table
+ *
+ * Copyright (C) 2012 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "%(prefix)s%(error_header)s"
+
+''',
+                prefix=prefix, error_header=error_header)
+    return ret
+
+def gen_error_macro(err_domain):
+    string = ''
+    cur = err_domain[0]
+    for nxt in err_domain[1:]:
+        if string and cur.isupper() and nxt.islower():
+            string += '_'
+        string += cur
+        cur = nxt
+    string += cur
+    return 'QERR_' + string.upper()
+
+def gen_error_def_table(exprs):
+    ret = mcgen('''
+static const QErrorStringTable qerror_table[] = {
+''')
+
+    for err in exprs:
+        macro = gen_error_macro(err['error'])
+        desc = err['description']
+        ret += mcgen('''
+        {
+            .error_fmt = %(error_macro)s,
+            .desc      = "%(error_desc)s",
+        },
+''',    
+                    error_macro=macro, error_desc=desc)
+
+    ret += mcgen('''
+    {}
+};
+''')
+
+    return ret
+
+def gen_error_macro_data_str(data):
+    colon = ''
+    data_str = ''
+    for k, v in data.items():
+        data_str += colon + "'%s': " % k
+        if v == 'str':
+            data_str += "%s"
+        elif v == 'int':
+            data_str += '%"PRId64"'
+        else:
+            sys.exit("unknown data type '%s' for error '%s'" % (v, name))
+        colon = ', '
+    return data_str
+
+def gen_error_decl_macros(exprs):
+    ret = ''
+    for err in exprs:
+        data = gen_error_macro_data_str({})
+        if err.has_key('data'):
+            data = gen_error_macro_data_str(err['data'])
+        macro = gen_error_macro(err['error'])
+        name = err['error']
+
+        ret += mcgen('''
+#define %(error_macro)s \\
+    "{ 'class': '%(error_class)s', 'data': { %(error_data)s } }"
+
+''',
+                error_macro=macro, error_class=name, error_data=data)
+    return ret
+
+def maybe_open(really, name, opt):
+    if really:
+        return open(name, opt)
+    else:
+        import StringIO
+        return StringIO.StringIO()
+
+if __name__ == '__main__':
+    try:
+        opts, args = getopt.gnu_getopt(sys.argv[1:], "chp:o:",
+                                       ["prefix=", "output-dir="])
+    except getopt.GetoptError, err:
+        print str(err)
+        sys.exit(1)
+
+    prefix = ""
+    output_dir = ""
+    do_c = True
+    do_h = True
+    c_file = 'qapi-errors.c'
+    h_file = 'qapi-errors.h'
+
+    for o, a in opts:
+        if o in ("-p", "--prefix"):
+            prefix = a
+        elif o in ("-o", "--output-dir"):
+            output_dir = a + "/"
+
+    c_file = output_dir + prefix + c_file
+    h_file = output_dir + prefix + h_file
+
+    try:
+        os.makedirs(output_dir)
+    except os.error, e:
+        if e.errno != errno.EEXIST:
+            raise
+
+    exprs = parse_schema(sys.stdin)
+
+    fdecl = maybe_open(do_h, h_file, 'w')
+    ret = gen_error_decl_prologue(header=basename(h_file), guard=guardname(h_file), prefix=prefix)
+    fdecl.write(ret)
+
+    ret = gen_error_decl_macros(exprs)
+    fdecl.write(ret)
+
+    fdecl.write("#endif\n")
+    fdecl.flush()
+    fdecl.close()
+
+    fdef = maybe_open(do_c, c_file, 'w')
+    ret = gen_error_def_prologue(error_header=h_file, prefix=prefix)
+    fdef.write(ret)
+
+    ret = gen_error_def_table(exprs)
+    fdef.write(ret)
+
+    fdef.flush()
+    fdef.close()