Re: [Qemu-devel] [PATCH V6 00/29] add direct support of event in qapi schema

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3832 bytes --]

On 06/17/2014 04:57 AM, Paolo Bonzini wrote:
> Il 15/06/2014 02:52, Wenchao Xia ha scritto:
>>> Unfortunately, this already does not apply anymore.
>>>
>>> I've placed the rebase on branch qapi-event of my github repository. The
>>> resolutions are trivial, so perhaps Luiz or Michael can pull from there?
>>>
>>> Paolo
>>
>>
>>   Thanks for testing my work:)
>>   Eric:
>>   I have looked the comments, most fix will arrive in my V7 next week,
>> but still I have two issues without decision:
>>   1. Whether to use QAPIEvent in callback function type declartion,
>> See my feedback in 6/29.
>>   2. Whether to make qapi-event.json self sufficent, see my comments
>> in 17/29.
> 
> I am afraid that this will miss 2.1 (and so will dataplane 
> rerror/werror that depends on it), so I went ahead and done all fixes in 
> my qapi-event branch.
> 
> I have not yet applied Reviewed-by tags from Eric since some patches
> are different and I want him to look at the interdiff first.

Here's my first glance through your interdiff; I will also respond once
I've looked at the commits leading to
8910d4c0f568d1eb46934ef16e9a275d97749973 (your current qapi-event branch
head at git://github.com/bonzini/qemu.git).

> 
> Regarding error handling, I have left the code in but switched all
> callers to &error_abort.

Works for me to use &error_abort.

> 
> The interdiff is here.  I ensured that the result is 
> bisectable, and the intermediate changes are responsible
> for the "spurious" hunks of the interdiff:
> 
> diff --git a/Makefile b/Makefile
> index 3e65525..f473cf5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -246,8 +246,7 @@ $(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-commands.py $(qapi-py)
>  		$(gen-out-type) -o "." -b -i $<, \
>  		"  GEN   $@")
>  qapi-event.c qapi-event.h :\
> -$(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi-event.json \
> -$(SRC_PATH)/scripts/qapi-event.py $(qapi-py)
> +$(qapi-modules) $(SRC_PATH)/scripts/qapi-event.py $(qapi-py)

Where's the change that adds qapi/event.json to $(qapi-modules)?  (Maybe
the interdiff doesn't show it?)


> +++ b/include/sysemu/os-posix.h
> @@ -26,8 +26,6 @@
>  #ifndef QEMU_OS_POSIX_H
>  #define QEMU_OS_POSIX_H
>  
> -#include <sys/time.h>
> -
>  void os_set_line_buffering(void);

Why this hunk? Did you accidentally forget to include Wenchao's 1/29 in
your series?

>  void os_set_proc_name(const char *s);
>  void os_setup_signal_handling(void);
> diff --git a/monitor.c b/monitor.c
> index 6b693ee..66a1db7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -183,10 +183,10 @@ typedef struct MonitorControl {
>   */
>  typedef struct MonitorQAPIEventState {
>      QAPIEvent event;    /* Event being tracked */
> -    int64_t rate;       /* Period over which to throttle. 0 to disable */
> -    int64_t last;       /* Time at which event was last emitted */
> +    int64_t rate;       /* Minimum time (in ns) between two events */
> +    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */

I still like the "0 to disable" comment; but I see that you dropped it
to keep line length manageable.  So I guess I can live with this change.

> @@ -535,14 +533,15 @@ static void monitor_qapi_event_handler(void *opaque)
>   * more than 1 event will be emitted within @rate
>   * milliseconds
>   */
> -static void monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
> +static void
> +monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>  {

Not the usual qemu formatting style.

Overall the interdiff looks like it resolves most of my concerns on v6.
 Now for me to read the actual commits in your git repo...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]