Re: [Qemu-devel] [PATCH v5 02/20] qapi.py: add a simple #ifdef conditional

[Marc-André Lureau]

Hi

On Tue, Sep 6, 2016 at 5:00 PM Markus Armbruster <address@hidden> wrote:

> Marc-André Lureau <address@hidden> writes:
>
> > Learn to parse #define files provided with -f option, and skip
> > undefined #ifdef blocks in the schema.
> >
> > This is a very simple pre-processing, without stacking support or
> > evaluation (it could be implemented if needed).
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  scripts/qapi.py | 43 ++++++++++++++++++++++++++++++++++++++++---
>
> Missing: update of docs/qapi-code-gen.txt.
>
> >  1 file changed, 40 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index 21bc32f..d0b8a66 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -76,6 +76,7 @@ struct_types = []
> >  union_types = []
> >  events = []
> >  all_names = {}
> > +defs = []
> >
> >  #
> >  # Parsing the schema into expressions
> > @@ -177,6 +178,7 @@ class QAPISchemaParser(object):
> >                  self.exprs.append(expr_elem)
> >
> >      def accept(self):
> > +        ok = True
> >          while True:
> >              self.tok = self.src[self.cursor]
> >              self.pos = self.cursor
> > @@ -184,7 +186,19 @@ class QAPISchemaParser(object):
> >              self.val = None
> >
> >              if self.tok == '#':
> > -                self.cursor = self.src.find('\n', self.cursor)
> > +                end = self.src.find('\n', self.cursor)
> > +                line = self.src[self.cursor:end+1]
> > +                self.cursor = end
> > +                sline = line.split()
> > +                if len(defs) and len(sline) >= 1 \
> > +                   and sline[0] in ['ifdef', 'endif']:
> > +                    if sline[0] == 'ifdef':
> > +                        ok = sline[1] in defs
> > +                    elif sline[0] == 'endif':
> > +                        ok = True
> > +                    continue
> > +            elif not ok:
> > +                continue
> >              elif self.tok in "{}:,[]":
> >                  return
> >              elif self.tok == "'":
>
> Oww, you're abusing comments!  That's horrible :)
>

Can we make this real syntax, like everything else, including 'include'?
>
>
We already abuse json, which doesn't support comments either. :) Even
without comments, our syntax is wrong and confuse most editors/validators.


> Unfortunately, the natural
>
>     { 'ifdef': 'CONFIG_FOO'
>       'then': ...   # ignored unless CONFIG_FOO
>       'else': ...   # ignored if CONFIG_FOO (optional)
>     }
>
> is awkward, because the ... have to be a single JSON value, but a QAPI
> schema isn't a single JSON value, it's a *sequence* of JSON values.  A
> top-level stanza
>
>     JSON-value1 JSON-value2 ...
>
> would become
>
>     [ JSON-value1, JSON-value2, ... ]
>
> within a conditional.  Blech.
>
> Could sacrifice the nesting and do
>
>     { 'ifdef': 'CONFIG_FOO' }
>     ...
>     { 'endif' }
>
> Widens the gap between syntax and semantics.  Editors can no longer
> easily jump over the conditional (e.g. Emacs forward-sexp).  Nested
> conditionals becomes as confusing as in C.  Not exactly elegant, either.
>
> Yet another option is to add 'ifdef' keys to the definitions
> themselves, i.e.
>
>     { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
>       'ifdef': 'TARGET_ARM' }
>

That's the worst of all options imho, as it makes it hard to filter out
unions/enums, ex:

 @ -3446,8 +3466,10 @@
                                        'testdev': 'ChardevCommon',
                                        'stdio'  : 'ChardevStdio',
                                        'console': 'ChardevCommon',
+#ifdef CONFIG_SPICE
                                        'spicevmc' : 'ChardevSpiceChannel',
                                        'spiceport' : 'ChardevSpicePort',
+#endif
                                        'vc'     : 'ChardevVC',
                                        'ringbuf': 'ChardevRingbuf',

I think #ifdef is the most straightforward/easy thing to do. My experience
with doc generations tells me that is really not an issue to reserve the
#\w* lines for pre-processing.

And it's a natural fit with the rest of qemu #if conditionals.




> > @@ -1707,15 +1721,36 @@ def gen_params(arg_type, boxed, extra):
> >  #
> >  # Common command line parsing
> >  #
> > +def defile(filename):
>
> From The Collaborative International Dictionary of English v.0.48 [gcide]:
>
> Defile \De*file"\ (d[-e]*f[imac]l"), v. t. [OE. defoulen,
>    -foilen, to tread down, OF. defouler; de- + fouler to trample
>    (see Full, v. t.), and OE. defoulen to foul (influenced in
>    form by the older verb defoilen). See File to defile,
>    Foul, Defoul.]
>    1. To make foul or impure; to make filthy; to dirty; to
>       befoul; to pollute.
>       [1913 Webster]
>
>             They that touch pitch will be defiled. --Shak.
>       [1913 Webster]
>
>    2. To soil or sully; to tarnish, as reputation; to taint.
>       [1913 Webster]
>
>             He is . . . among the greatest prelates of this age,
>             however his character may be defiled by . . . dirty
>             hands.                                --Swift.
>       [1913 Webster]
>
>    3. To injure in purity of character; to corrupt.
>       [1913 Webster]
>
>             Defile not yourselves with the idols of Egypt.
>                                                   --Ezek. xx. 7.
>       [1913 Webster]
>
>    4. To corrupt the chastity of; to debauch; to violate; to
>       rape.
>       [1913 Webster]
>
>             The husband murder'd and the wife defiled. --Prior.
>       [1913 Webster]
>
>    5. To make ceremonially unclean; to pollute.
>       [1913 Webster]
>
>             That which dieth of itself, or is torn with beasts,
>             he shall not eat to defile therewith. --Lev. xxii.
>                                                   8.
>       [1913 Webster]
>
> Fitting in a way; you're defiling the poor, innocent comment syntax ;)
>

ok


>
> > +    f = open(filename, 'r')
> > +    while 1:
>
> while True:
>
> > +        line = f.readline()
> > +        if not line:
> > +            break
> > +        while line[-2:] == '\\\n':
> > +            nextline = f.readline()
> > +            if not nextline:
> > +                break
> > +            line = line + nextline
> > +        tmp = line.strip()
> > +        if tmp[:1] != '#':
> > +            continue
> > +        tmp = tmp[1:]
> > +        words = tmp.split()
> > +        if words[0] != "define":
> > +            continue
> > +        defs.append(words[1])
> > +    f.close()
>
> This parses Yet Another Language.  Worse, Yet Another Undocumented
> Language.  Why not JSON?
>

> Hmm, peeking ahead to PATCH 04... aha!  This is for reading
> config-host.h and config-target.h.  So, this actually doesn't parse
> YAUL, it parses C.  Sloppily, of course.
>

Yes


>
> I guess we could instead parse config-host.mak and config-target.mak
> sloppily.  Not sure which idea is more disgusting :)
>
> Could we punt evaluating conditionals to the C compiler?  Instead of
> emitting TEXT when CONFIG_FOO is defined, emit
>
>     #ifdef CONFIG_FOO
>     TEXT
>     #endif
>
>
I don't follow you


> >
> >
> >  def parse_command_line(extra_options="", extra_long_options=[]):
> >
> >      try:
> >          opts, args = getopt.gnu_getopt(sys.argv[1:],
> > -                                       "chp:o:" + extra_options,
> > +                                       "chp:o:f:" + extra_options,
> >                                         ["source", "header", "prefix=",
> > -                                        "output-dir="] +
> extra_long_options)
> > +                                        "output-dir=", "--defile="] +
>
> https://docs.python.org/3/library/getopt.html on the third argument:
> "The leading '--' characters should not be included in the option name."
>

ok


>
> > +                                       extra_long_options)
> >      except getopt.GetoptError as err:
> >          print >>sys.stderr, "%s: %s" % (sys.argv[0], str(err))
> >          sys.exit(1)
> > @@ -1742,6 +1777,8 @@ def parse_command_line(extra_options="",
> extra_long_options=[]):
> >              do_c = True
> >          elif o in ("-h", "--header"):
> >              do_h = True
> > +        elif o in ("-f", "--defile"):
> > +            defile(a)
> >          else:
> >              extra_opts.append(oa)
>
> --
Marc-André Lureau