Re: [Qemu-devel] [PATCH V6 17/29] qapi event: convert WATCHDOG

[Wenchao Xia]

于 2014/6/14 6:05, Eric Blake 写道:
> On 06/13/2014 03:47 PM, Eric Blake wrote:
> > On 06/05/2014 06:22 AM, Wenchao Xia wrote:
> > > Signed-off-by: Wenchao Xia <address@hidden>
> > > ---
> > >   docs/qmp/qmp-events.txt |   19 -------------------
> > >   hw/watchdog/watchdog.c  |   23 +++++++----------------
> > >   monitor.c               |    2 +-
> > >   qapi-event.json         |   15 +++++++++++++++
> > >   qapi-schema.json        |   24 ++++++++++++++++++++++++
> > >   5 files changed, 47 insertions(+), 36 deletions(-)
>
> > > +  'data': { 'action': 'WatchdogExpirationAction' } }
> >
> > Hmm.  You've managed to create error.json in such a manner that it is
> > not self-sufficient.  If some other file includes error.json, it must
> > also define WatchdogExpirationAction or it will fail the generators.
>
> s/error.json/qapi-event.json/
>
> > > +++ b/qapi-schema.json
> >
> > > +##
> > > +# @WatchdogExpirationAction
> >
> > I think you will be better off to ensure that error.json is
>
> and again qapi-event.json (not sure why I typed error.json).
>
> > self-sufficient, perhaps by sticking any data type it references
> > directly into common.json rather than qapi-schema.json, and having
> > error.json include common.json.  (This is the first instance of
> > referencing an external type, but other events later in the series have
> > the same issue).
>
> Oh weird! I've managed to run all four of
> scripts/qapi-{visit,types,commands,event}.py directly on
> qapi-event.json, and didn't get any complaints from the generator.  BUT,
> the generated code is definitely different:
>
> -void qapi_event_send_watchdog(WatchdogExpirationAction action,
> +void qapi_event_send_watchdog(WatchdogExpirationAction * action,
>                                 Error **errp)
>
> That is, when the enum type is known (because the parse was done on
> qapi-schema.json), the WatchdogExpirationAction argument is treated as
> an integer enum value; but when the enum type is unknown (because the
> parse was done directly on an incomplete qapi-event.json), the generator
> tries to treat it as a pointer to an otherwise unknown structure.
> (Never mind the odd formatting of the space after the '*' - I believe
> this pending patch fixes it:
> https://lists.gnu.org/archive/html/qemu-devel/2014-06/msg02385.html).
>
> This little exercise raises red flags for me - we probably ought to
> enhance the code generators to error out instead of blindly act as if
> unknown types are pointers.  But save it for another day - no need to
> stall this series just to wait for a robustness improvement to the
> generators.


> Meanwhile, my suggestion of making qapi-event.json to be self-sufficient
> is going to be a bit harder to test, but is still probably worth trying
> (just moving common types into a common shared include).

  I think it is a issue about how to orgnize the .json files. There are
some common type defines needed for different .json files, in my series
they are needed both in qapi-schema.json and qapi-event.json. So to
make qapi-event.json self-sufficient, qapi-schema.json will be
insufficient. I considered this before, and thought it is better
to reorgnize .json files as:

      qapi-types.json
      |             |
qapi-cmd.json    qapi-event.json

  It is an adjusting work for existing code, So I didn't do that in my
series.