Re: [Qemu-devel] [PATCH 01/17] qidl: add QEMU IDL processor

[Anthony Liguori <aliguori@us.ibm.com>]

Hi,

Thanks for sending this Mike.  If you're on CC, please do read the qc.md at 
least.  There is a good bit of theory here about how to rigorously approach 
serialization of device state.  I think the approach is sound and could solve a 
lot of the problems we face today but it could use review.

On 06/05/2012 09:00 AM, Michael Roth wrote:
> This is an import of Anthony's qidl compiler, with some changes squashed
> in to add support for doing the visitor generation via QEMU's qapi code
> generators rather than directly.
>
> Documentation has been imported as well, as is also viewable at:
>
> https://github.com/aliguori/qidl/blob/master/qc.md
>
> This will be used to add annotations to device structs to aid in
> generating visitors that can be used to serialize/unserialize them.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   scripts/qc.md |  331 ++++++++++++++++++++++++++++++++++++++
>   scripts/qc.py |  494 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 825 insertions(+), 0 deletions(-)
>   create mode 100644 scripts/qc.md
>   create mode 100755 scripts/qc.py
>
> diff --git a/scripts/qc.md b/scripts/qc.md
> new file mode 100644
> index 0000000..4cf4b21
> --- /dev/null
> +++ b/scripts/qc.md
> @@ -0,0 +1,331 @@
> +How to Serialize Device State with QC
> +======================================
> +
> +This document describes how to implement save/restore of a device in QEMU using
> +the QC IDL compiler.  The QC IDL compiler makes it easier to support live
> +migration in devices by converging the serialization description with the
> +device type declaration.  It has the following features:
> +
> + 1. Single description of device state and how to serialize
> +
> + 2. Fully inclusive serialization description--fields that aren't serialized
> +    are explicitly marked as such including the reason why.
> +
> + 3. Optimized for the common case.  Even without any special annotations,
> +    many devices will Just Work out of the box.
> +
> + 4. Build time schema definition.  Since QC runs at build time, we have full
> +    access to the schema during the build which means we can fail the build if
> +    the schema breaks.
> +
> +For the rest, of the document, the following simple device will be used as an
> +example.
> +
> +    typedef struct SerialDevice {
> +        SysBusDevice parent;
> +
> +        uint8_t thr;            // transmit holding register
> +        uint8_t lsr;            // line status register
> +        uint8_t ier;            // interrupt enable register
> +
> +        int int_pending;        // whether we have a pending queued interrupt
> +        CharDriverState *chr;   // backend
> +    } SerialDevice;
> +
> +Getting Started
> +---------------
> +
> +The first step is to move your device struct definition to a header file.  This
> +header file should only contain the struct definition and any preprocessor
> +declarations you need to define the structure.  This header file will act as
> +the source for the QC IDL compiler.
> +
> +Do not include any function declarations in this header file as QC does not
> +understand function declarations.

This is out of date I think.  I believe that QC now supports parsing function 
declarations.

> +Determining What State Gets Saved
> +---------------------------------
> +
> +By default, QC saves every field in a structure it sees.  This provides maximum
> +correctness by default.  However, device structures generally contain state
> +that reflects state that is in someway duplicated or not guest visible.  This
> +more often that not reflects design implementation details.
> +
> +Since design implementation details change over time, saving this state makes
> +compatibility hard to maintain since it would effectively lock down a device's
> +implementation.
> +
> +QC allows a device author to suppress certain fields from being saved although
> +there are very strict rules about when this is allowed and what needs to be done
> +to ensure that this does not impact correctness.
> +
> +There are three cases where state can be suppressed: when it is **immutable**,
> +**derived**, or **broken**.  In addition, QC can decide at run time whether to
> +suppress a field by assigning it a **default** value.
> +
> +## Immutable Fields
> +
> +If a field is only set during device construction, based on parameters passed to
> +the device's constructor, then there is no need to send save and restore this
> +value.  We call these fields immutable and we tell QC about this fact by using
> +a **_immutable** marker.
> +
> +In our *SerialDevice* example, the *CharDriverState* pointer reflects the host
> +backend that we use to send serial output to the user.  This is only assigned
> +during device construction and never changes.  This means we can add an
> +**_immutable** marker to it:
> +
> +    typedef struct SerialDevice {
> +        SysBusDevice parent;
> +
> +        uint8_t thr;            // transmit holding register
> +        uint8_t lsr;            // line status register
> +        uint8_t ier;            // interrupt enable register
> +
> +        int int_pending;        // whether we have a pending queued interrupt
> +        CharDriverState _immutable *chr;
> +    } SerialDevice;
> +
> +When reviewing patches that make use of the **_immutable** marker, the following
> +guidelines should be followed to determine if the marker is being used
> +correctly.
> +
> + 1. Check to see if the field is assigned anywhere other than the device
> +    initialization function.
> +
> + 2. Check to see if any function is being called that modifies the state of the
> +    field outside of the initialization function.
> +
> +It can be subtle whether a field is truly immutable.  A good example is a
> +*QEMUTimer*.  Timer's will usually have their timeout modified with a call to
> +*qemu_mod_timer()* even though they are only assigned in the device
> +initialization function.
> +
> +If the timer is always modified with a fixed value that is not dependent on
> +guest state, then the timer is immutable since it's unaffected by the state of
> +the guest.
> +
> +On the other hand, if the timer is modified based on guest state (such as a
> +guest programmed time out), then the timer carries state.  It may be necessary
> +to save/restore the timer or mark it as **_derived** and work with it
> +accordingly.
> +
> +### Derived Fields
> +
> +If a field is set based on some other field in the device's structure, then its
> +value is derived.  Since this is effectively duplicate state, we can avoid
> +sending it and then recompute it when we need to.  Derived state requires a bit
> +more handling that immutable state.
> +
> +In our *SerialDevice* example, our *int_pending* flag is really derived from
> +two pieces of state.  It is set based on whether interrupts are enabled in the
> +*ier* register and whether there is *THRE* flag is not set in the *lsr*
> +register.
> +
> +To mark a field as derived, use the **_derived** marker.  To update our
> +example, we would do:
> +
> +    typedef struct SerialDevice {
> +        SysBusDevice parent;
> +
> +        uint8_t thr;            // transmit holding register
> +        uint8_t lsr;            // line status register
> +        uint8_t ier;            // interrupt enable register
> +
> +        int _derived int_pending; // whether we have a pending queued interrupt
> +        CharDriverState _immutable *chr;
> +    } SerialDevice;
> +
> +There is one other critical step needed when marking a field as derived.  A
> +*post_load* function must be added that updates this field after loading the
> +rest of the device state.  This function is implemented in the device's source
> +file, not in the QC header.  Below is an example of what this function may do:
> +
> +    static void serial_post_load(SerialDevice *s)
> +    {
> +        s->int_pending = !(s->lsr&  THRE)&&  (s->ier&  INTE);
> +    }
> +
> +When reviewing a patch that marks a field as *_derived*, the following criteria
> +should be used:
> +
> + 1. Does the device have a post load function?
> +
> + 2. Does the post load function assign a value to all of the derived fields?
> +
> + 3. Are there any obvious places where a derived field is holding unique state?
> +
> +### Broken State
> +
> +QEMU does migration with a lot of devices today.  When applying this methodology
> +to these devices, one will quickly discover that there are a lot of fields that
> +are not being saved today that are not derived or immutable state.
> +
> +These are all bugs.  It just so happens that these bugs are usually not very
> +serious.  In many cases, they cause small functionality glitches that so far
> +have not created any problems.
> +
> +Consider our *SerialDevice* example.  In QEMU's real *SerialState* device, the
> +*thr* register is not saved yet we have not marked it immutable or derived.
> +
> +The *thr* register is a temporary holding register that the next character to
> +transmit is placed in while we wait for the next baud cycle.  In QEMU, we
> +emulate a very fast baud rate regardless of what guest programs.  This means
> +that the contents of the *thr* register only matter for a very small period of
> +time (measured in microseconds).
> +
> +The likelihood of a migration converging in that very small period of time when
> +the *thr* register has a meaningful value is very small.  Moreover, the worst
> +thing that can happen by not saving this register is that we lose a byte in the
> +data stream.  Even if this has happened in practice, the chances of someone
> +noticing this as a bug is pretty small.
> +
> +Nonetheless, this is a bug and needs to be eventually fixed.  However, it would
> +be very inconvenient to constantly break migration by fixing all of these bugs
> +one-by-one.  Instead, QC has a **_broken** marker.  This indicates that a field
> +is not currently saved, but should be in the future.
> +
> +The idea behind the broken marker is that we can convert a large number of
> +devices without breaking migration compatibility, and then institute a flag day
> +where we go through and remove broken markers en-mass.
> +
> +Below is an update of our example to reflect our real life serial device:
> +
> +    typedef struct SerialDevice {
> +        SysBusDevice parent;
> +
> +        uint8_t _broken thr;    // transmit holding register
> +        uint8_t lsr;            // line status register
> +        uint8_t ier;            // interrupt enable register
> +
> +        int _derived int_pending; // whether we have a pending queued interrupt
> +        CharDriverState _immutable *chr;
> +    } SerialDevice;
> +
> +When reviewing the use of the broken marker, the following things should be
> +considered:
> +
> + 1. What are the ramifications of not sending this data field?
> +
> + 2. If the not sending this data field can cause data corruption or very poor
> +    behavior within the guest, the broken marker is not appropriate to use.
> +
> + 3. Assigning a default value to a field can also be used to fix a broken field
> +    without significantly impacting live migration compatibility.
> +
> +### Default Values
> +
> +In many cases, a field that gets marked broken was not originally saved because
> +in the vast majority of the time, the field does not contain a meaningful value.
> +
> +In the case of our *thr* example, the field usually does not have a meaningful
> +value.
> +
> +Instead of always saving the field, QC has another mechanism that allows the
> +field to be saved only when it has a meaningful value.  This is done using the
> +**_default()** marker.  The default marker tells QC that if the field currently
> +has a specific value, do not save the value as part of serialization.
> +
> +When loading a field, QC will assign the default value to the field before it
> +tries to load the field.  If the field cannot be loaded, QC will ignore the
> +error and rely on the default value.
> +
> +Using default values, we can fix broken fields while also minimizing the cases
> +where we break live migration compatibility.  The **_default()** marker can be
> +used in conjunction with the **_broken** marker.  We can extend our example as
> +follows:
> +
> +    typedef struct SerialDevice {
> +        SysBusDevice parent;
> +
> +
> +        uint8_t thr _default(0); // transmit holding register
> +        uint8_t lsr;             // line status register
> +        uint8_t ier;             // interrupt enable register
> +
> +        int _derived int_pending; // whether we have a pending queued interrupt
> +        CharDriverState _immutable *chr;
> +    } SerialDevice;
> +
> +The following guidelines should be followed when using a default marker:
> +
> + 1. Is the field set to the default value both during device initialization and
> +    whenever the field is no longer in use?
> +
> + 2. If the non-default value is expected to occur often, then consider using the
> +    **_broken** marker along with the default marker and using a flag day to
> +    remove the **_broken** marker.
> +
> + 3. In general, setting default values as the value during device initialization
> +    is a good idea even if the field was never broken.  This gives us maximum
> +    flexibility in the long term.
> +
> + 4. Never change a default value without renaming a field.  The default value is
> +    part of the device's ABI.
> +
> +The first guideline is particularly important.  In the case of QEMU's real
> +*SerialDevice*, it would be necessary to add code to set the *thr* register to
> +zero after the byte has been successfully transmitted.  Otherwise, it is
> +unlikely that it would ever contain the default value.
> +
> +Arrays
> +------
> +
> +QC has support for multiple types of arrays.  The following sections describe
> +the different rules for arrays.
> +
> +Fixed Sized Arrays
> +------------------
> +
> +A fixed sized array has a size that is known at build time.  A typical example
> +would be:
> +
> +    struct SerialFIFO {
> +        uint8_t data[UART_FIFO_LENGTH];
> +        uint8_t count;
> +        uint8_t itl;
> +        uint8_t tail;
> +        uint8_t head;
> +    };
> +
> +In this example, *data* is a fixed sized array.  No special annotation is needed
> +for QC to marshal this area correctly.  The following guidelines apply to
> +fixed sized arrays:
> +
> + 1. The size of the array is part of the device ABI.  It should not change
> +    without renaming the field.
> +
> +Variable Sized, Fixed Capacity Arrays
> +-------------------------------------
> +
> +Sometimes it's desirable to have a variable sized array.  QC currently supported
> +variable sized arrays provided that the maximum capacity is fixed and part of
> +the device structure memory.
> +
> +A typical example would be a slightly modified version of our above example:
> +
> +    struct SerialFIFO {
> +        uint8_t count;
> +        uint8_t _size_is(count) data[UART_FIFO_LENGTH];
> +        uint8_t itl;
> +        uint8_t tail;
> +        uint8_t head;
> +    };
> +
> +In this example, *data* is a variable sized array with a fixed capacity of
> +*UART_FIFO_LENGTH*.  When we serialize, we want only want to serialize *count*
> +members.
> +
> +The ABI implications of capacity are a bit more relaxed with variable sized
> +arrays.  In general, you can increase or decrease the capacity without breaking
> +the ABI although you may cause some instances of migration to fail between
> +versions of QEMU with different capacities.
> +
> +When reviewing variable sized, fixed capacity arrays, keep the following things
> +in mind:
> +
> + 1. The variable size must occur before the array element in the state
> +    structure.
> +
> + 2. The capacity can change without breaking the ABI, but care should be used
> +    when making these types of changes.

For future patches, please split the README from the script.

> diff --git a/scripts/qc.py b/scripts/qc.py
> new file mode 100755
> index 0000000..74f2a40
> --- /dev/null
> +++ b/scripts/qc.py
> @@ -0,0 +1,494 @@

Needs a copyright.

> +#!/usr/bin/python
> +
> +import sys
> +from ordereddict import OrderedDict
> +
> +marker = "qc_declaration"
> +marked = False
> +
> +class Input(object):
> +    def __init__(self, fp):
> +        self.fp = fp
> +        self.buf = ''
> +        self.eof = False
> +
> +    def pop(self):
> +        if len(self.buf) == 0:
> +            if self.eof:
> +                return ''
> +
> +            data = self.fp.read(1024)
> +            if data == '':
> +                self.eof = True
> +                return ''
> +
> +            self.buf += data
> +
> +        ch = self.buf[0]
> +        self.buf = self.buf[1:]
> +        return ch
> +
> +def in_range(ch, start, end):
> +    if ch>= start and ch<= end:
> +        return True
> +    return False
> +
> +# D			[0-9]
> +# L			[a-zA-Z_]
> +# H			[a-fA-F0-9]
> +# E			[Ee][+-]?{D}+
> +# FS			(f|F|l|L)
> +# IS			(u|U|l|L)*
> +
> +def is_D(ch):
> +    return in_range(ch, '0', '9')
> +
> +def is_L(ch):
> +    return in_range(ch, 'a', 'z') or in_range(ch, 'A', 'Z') or ch == '_'
> +
> +def is_H(ch):
> +    return in_range(ch, 'a', 'f') or in_range(ch, 'A', 'F') or is_D(ch)
> +
> +def is_FS(ch):
> +    return ch in 'fFlL'
> +
> +def is_IS(ch):
> +    return ch in 'uUlL'
> +
> +def find_marker(ch, fp):
> +    global marked
> +
> +    # scan for marker before full processing
> +
> +    while not marked and not fp.eof:
> +        token = ''
> +        if is_L(ch):
> +            token += ch
> +            while True:
> +                ch = fp.pop()
> +                if not is_L(ch) and not is_D(ch):
> +                    break
> +                token += ch
> +            if token == marker:
> +                marked = True
> +                return
> +        ch = fp.pop()
> +    return
> +
> +def lexer(fp):
> +    global marked
> +    ch = fp.pop()
> +
> +    while not fp.eof:
> +        if not marked:
> +            find_marker(ch, fp)
> +            ch = fp.pop()
> +        token = ''
> +
> +        if is_L(ch):
> +            token += ch
> +
> +            ch = fp.pop()
> +            while is_L(ch) or is_D(ch):
> +                token += ch
> +                ch = fp.pop()
> +            if token in [ 'auto', 'break', 'case', 'const', 'continue',
> +                           'default', 'do', 'else', 'enum', 'extern',
> +                           'for', 'goto', 'if', 'register', 'return', 'signed',
> +                           'sizeof',
> +                           'static', 'struct', 'typedef', 'union', 'unsigned',
> +                           'void', 'volatile', 'while' ]:
> +                yield (token, token)
> +            else:
> +                yield ('symbol', token)
> +        elif ch == "'":
> +            token += ch
> +
> +            ch = fp.pop()
> +            if ch == '\\':
> +                token += ch
> +                token += fp.pop()
> +            else:
> +                token += ch
> +            token += fp.pop()
> +            ch = fp.pop()
> +            yield ('literal', token)
> +        elif ch == '"':
> +            token += ch
> +
> +            ch = fp.pop()
> +            while ch not in ['', '"']:
> +                token += ch
> +                if ch == '\\':
> +                    token += fp.pop()
> +                ch = fp.pop()
> +            token += ch
> +            yield ('literal', token)
> +            ch = fp.pop()
> +        elif ch in '.><+-*/%&^|!;{},:=()[]~?':
> +            token += ch
> +            ch = fp.pop()
> +            tmp_token = token + ch
> +            if tmp_token in ['<:']:
> +                yield ('operator', '[')
> +                ch = fp.pop()
> +            elif tmp_token in [':>']:
> +                yield ('operator', ']')
> +                ch = fp.pop()
> +            elif tmp_token in ['<%']:
> +                yield ('operator', '{')
> +                ch = fp.pop()
> +            elif tmp_token in ['%>']:
> +                yield ('operator', '}')
> +                ch = fp.pop()
> +            elif tmp_token == '//':
> +                token = tmp_token
> +                ch = fp.pop()
> +                while ch != '\n' and ch != '':
> +                    token += ch
> +                    ch = fp.pop()
> +                yield ('comment', token)
> +            elif tmp_token == '/*':
> +                token = tmp_token
> +
> +                ch = fp.pop()
> +                while True:
> +                    while ch != '*':
> +                        token += ch
> +                        ch = fp.pop()
> +                    token += ch
> +                    ch = fp.pop()
> +                    if ch == '/':
> +                        token += ch
> +                        break
> +                yield ('comment', token)
> +                ch = fp.pop()
> +            elif tmp_token in [ '+=', '-=', '*=', '/=', '%=', '&=', '^=',
> +                                '|=', '>>','<<', '++', '--', '->','&&',
> +                                '||', '<=', '>=', '==', '!=' ]:
> +                yield ('operator', tmp_token)
> +                ch = fp.pop()
> +            else:
> +                yield ('operator', token)
> +        elif ch == '0':
> +            token += ch
> +            ch = fp.pop()
> +            if ch in 'xX':
> +                token += ch
> +                ch = fp.pop()
> +                while is_H(ch):
> +                    token += ch
> +                    ch = fp.pop()
> +                while is_IS(ch):
> +                    token += ch
> +                    ch = fp.pop()
> +            elif is_D(ch):
> +                token += ch
> +                ch = fp.pop()
> +                while is_D(ch):
> +                    token += ch
> +                    ch = fp.pop()
> +            yield ('literal', token)
> +        elif is_D(ch):
> +            token += ch
> +            ch = fp.pop()
> +            while is_D(ch):
> +                token += ch
> +                ch = fp.pop()
> +            yield ('literal', token)
> +        elif ch in ' \t\v\n\f':
> +            token += ch
> +            ch = fp.pop()
> +            while len(ch) and ch in ' \t\v\n\f':
> +                token += ch
> +                ch = fp.pop()
> +            yield ('whitespace', token)
> +        elif ch in '#':
> +            token += ch
> +            ch = fp.pop()
> +            while len(ch) and ch != '\n':
> +                token += ch
> +                ch = fp.pop()
> +            yield ('directive', token)
> +        else:
> +            yield ('unknown', ch)
> +            ch = fp.pop()

This lexer is based off of http://www.lysator.liu.se/c/ANSI-C-grammar-l.html

It would be good to put a reference to this in the code.

> +class LookAhead(object):
> +    def __init__(self, container):
> +        self.i = container.__iter__()
> +        self.la = []
> +        self.full = False
> +
> +    def at(self, i):
> +        if not marked:
> +            self.la = []
> +            self.full = False
> +        if i>= len(self.la):
> +            if self.full:
> +                raise StopIteration()
> +            else:
> +                try:
> +                    self.la.append(self.i.next())
> +                except StopIteration, e:
> +                    self.full = True
> +                    raise StopIteration()
> +
> +        return self.la[i]
> +
> +    def eof(self):
> +        try:
> +            self.at(len(self.la))
> +        except StopIteration, e:
> +            return True
> +
> +        return False
> +
> +def skip(c):
> +    for token, value in c:
> +        if token in ['whitespace', 'comment', 'directive']:
> +            continue
> +        yield (token, value)
> +
> +def expect(la, index, first, second=None):
> +    if la.at(index)[0] != first:
> +        raise Exception("expected '%s', got %s %s" % (first, la.at(index)[0], la.at(index)[1]))
> +    if second != None:
> +        if la.at(index)[1] != second:
> +            raise Exception("expected '%s', got %s" % (second, la.at(index)[1]))
> +    return index + 1, la.at(index)[1]
> +
> +def choice(la, index, first, second=None):
> +    if la.at(index)[0] != first:
> +        return False
> +    if second != None:
> +        if la.at(index)[1] != second:
> +            return False
> +    return True
> +
> +def parse_type(la, index):
> +    next = index
> +
> +    typename = ''
> +    if choice(la, next, 'struct', 'struct'):
> +        typename = 'struct '
> +        next += 1
> +
> +    next, rest = expect(la, next, 'symbol')
> +    typename += rest
> +
> +    ret = { 'type': typename }
> +
> +    if choice(la, next, 'symbol', '_derived'):
> +        next += 1
> +        ret['is_derived'] = True
> +    elif choice(la, next, 'symbol', '_immutable'):
> +        next += 1
> +        ret['is_immutable'] = True
> +    elif choice(la, next, 'symbol', '_broken'):
> +        next += 1
> +        ret['is_broken'] = True
> +    elif choice(la, next, 'symbol', '_version'):
> +        next += 1
> +
> +        next, _ = expect(la, next, 'operator', '(')
> +        next, version = expect(la, next, 'literal')
> +        next, _ = expect(la, next, 'operator', ')')
> +
> +        ret['version'] = version
> +    elif choice(la, next, 'symbol', '_size_is'):
> +        next += 1
> +
> +        next, _ = expect(la, next, 'operator', '(')
> +        next, array_size = expect(la, next, 'symbol')
> +        next, _ = expect(la, next, 'operator', ')')
> +
> +        ret['is_array'] = True
> +        ret['array_size'] = array_size
> +
> +    if choice(la, next, 'operator', '*'):
> +        next += 1
> +        ret['is_pointer'] = True
> +
> +    next, variable = expect(la, next, 'symbol')
> +    ret['variable'] = variable
> +
> +    if choice(la, next, 'operator', '['):
> +        next += 1
> +
> +        if not ret.has_key('is_array'):
> +            ret['is_array'] = True
> +            ret['array_size'] = la.at(next)[1]
> +        else:
> +            ret['array_capacity'] = la.at(next)[1]
> +        next += 1
> +
> +        next, _ = expect(la, next, 'operator', ']')
> +
> +    if choice(la, next, 'symbol', '_default'):
> +        next += 1
> +
> +        next, _ = expect(la, next, 'operator', '(')
> +        next, default = expect(la, next, 'literal')
> +        next, _ = expect(la, next, 'operator', ')')
> +
> +        ret['default'] = default
> +
> +    next, _ = expect(la, next, 'operator', ';')
> +
> +    return (next - index), ret
> +
> +def parse_struct(la, index):
> +    next = index
> +
> +    next, _ = expect(la, next, 'struct', 'struct')
> +
> +    name = None
> +    if choice(la, next, 'symbol'):
> +        name = la.at(next)[1]
> +        next += 1
> +
> +    next, _ = expect(la, next, 'operator', '{')
> +
> +    nodes = []
> +
> +    while not choice(la, next, 'operator', '}'):
> +        offset, node = parse_type(la, next)
> +        next += offset
> +        nodes.append(node)
> +
> +    next += 1
> +
> +    return (next - index), { 'struct': name, 'fields': nodes }
> +
> +def parse_typedef(la, index):
> +    next = index
> +
> +    next, _ = expect(la, next, 'typedef', 'typedef')
> +
> +    offset, node = parse_struct(la, next)
> +    next += offset
> +
> +    next, typename = expect(la, next, 'symbol')
> +
> +    return (next - index), { 'typedef': typename, 'type': node }

I think the rest should be a separate .py file.

> +
> +def qapi_format(node, is_save=True):
> +    if node.has_key('typedef'):
> +        dtype = node['typedef']
> +        fields = node['type']['fields']
> +    else:
> +        dtype = node['struct']
> +        fields = node['fields']
> +
> +    if is_save:
> +        print 'void qc_save_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
> +    else:
> +        print 'void qc_load_%s(Visitor *v, %s *s, const char *name, Error **errp)' % (dtype, dtype)
> +    print '{'
> +    print '    visit_start_struct(v, "%s", name, errp);' % (dtype)
> +    for field in fields:
> +        if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
> +            continue
> +
> +        if field['type'].endswith('_t'):
> +            typename = field['type'][:-2]
> +        else:
> +            typename = field['type']
> +
> +        if field.has_key('is_array'):
> +            if field.has_key('array_capacity'):
> +                print '    if (%(array_size)s>  %(array_capacity)s) {' % field
> +                print '        error_set(errp, QERR_FAULT, "Array size greater than capacity.");'
> +                print '    }'
> +                print '    %(array_size)s = MIN(%(array_size)s, %(array_capacity)s);' % field
> +            print '    visit_start_array(v, "%s", errp);' % (field['variable'])
> +            print '    for (size_t i = 0; i<  %s; i++) {' % (field['array_size'])
> +            print '        visit_type_%s(v,&s->%s[i], NULL, errp);' % (typename, field['variable'])
> +            print '    }'
> +            print '    visit_end_array(v, errp);'
> +        elif field.has_key('default'):
> +            if is_save:
> +                print '    if (s->%s != %s) {' % (field['variable'], field['default'])
> +                print '        visit_type_%s(v,&s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
> +                print '    }'
> +            else:
> +                print '    s->%s = %s;' % (field['variable'], field['default'])
> +                print '    visit_type_%s(v,&s->%s, "%s", NULL);' % (typename, field['variable'], field['variable'])
> +        else:
> +            print '    visit_type_%s(v,&s->%s, "%s", errp);' % (typename, field['variable'], field['variable'])
> +    print '    visit_end_struct(v, errp);'
> +    print '}'
> +    print
> +
> +import json
> +
> +def type_dump(node):
> +    print json.dumps(node, sort_keys=True, indent=4)
> +
> +def qapi_schema(node):
> +    schema = OrderedDict()
> +    data = OrderedDict()
> +    fields = None
> +    if node.has_key('typedef'):
> +        schema['type'] = node['typedef']
> +        fields = node['type']['fields']
> +    elif node.has_key('struct'):
> +        schema['type'] = node['struct']
> +        fields = node['fields']
> +    else:
> +        raise Exception("top-level neither typedef nor struct")
> +
> +    for field in fields:
> +        if field.has_key('is_derived') or field.has_key('is_immutable') or field.has_key('is_broken'):
> +            continue
> +
> +        description = {}
> +
> +        if field['type'].endswith('_t'):
> +            typename = field['type'][:-2]
> +        elif field['type'].startswith('struct '):
> +            typename = field['type'].split(" ")[1]
> +        else:
> +            typename = field['type']
> +
> +        if field.has_key('is_array') and field['is_array']:
> +            description['type'] = [typename]
> +            description['<annotated>'] = 'true'
> +            if field.has_key('array_size'):
> +                description['array_size'] = field['array_size']
> +            if field.has_key('array_capacity'):
> +                description['array_capacity'] = field['array_capacity']
> +        else:
> +            #description['type'] = typename
> +            description = typename
> +
> +        data[field['variable']] = description
> +
> +    schema['data'] = data
> +    print json.dumps(schema).replace("\"", "'")
> +
> +
> +if __name__ == '__main__':

Should be in a main() function.

Regards,

Anthony Liguori

> +    la = LookAhead(skip(lexer(Input(sys.stdin))))
> +
> +    index = 0
> +    while True:
> +        try:
> +            if choice(la, index, 'typedef'):
> +                offset, node = parse_typedef(la, index)
> +            elif choice(la, index, 'struct'):
> +                offset, node = parse_struct(la, index)
> +            else:
> +                continue
> +
> +            index, _ = expect(la, index + offset, 'operator', ';')
> +            marked = False
> +            index = 0
> +        except StopIteration, e:
> +            break
> +
> +        #qapi_format(node, True)
> +        #qapi_format(node, False)
> +        #type_dump(node)
> +        qapi_schema(node)