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

[Wenchao Xia <wenchaoqemu@gmail.com>]

于 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 <wenchaoqemu@gmail.com>
>>> ---
>>>   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.