From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60240) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwvtP-0001aM-Ds for qemu-devel@nongnu.org; Tue, 17 Jun 2014 12:05:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WwvtG-0004FP-3X for qemu-devel@nongnu.org; Tue, 17 Jun 2014 12:05:51 -0400 Received: from qmta01.emeryville.ca.mail.comcast.net ([2001:558:fe2d:43:76:96:30:16]:47881) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WwvtF-0004Em-Nm for qemu-devel@nongnu.org; Tue, 17 Jun 2014 12:05:42 -0400 Message-ID: <53A06752.4060206@redhat.com> Date: Tue, 17 Jun 2014 10:05:38 -0600 From: Eric Blake MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <53969C25.7010705@redhat.com> <539CEE42.5090906@gmail.com> <53A01F06.5000209@redhat.com> In-Reply-To: <53A01F06.5000209@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="D18nW59X0MoGdU6rQa4wVI8OD8AAKKgRe" Subject: Re: [Qemu-devel] [PATCH V6 00/29] add direct support of event in qapi schema List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , Wenchao Xia , qemu-devel@nongnu.org Cc: lcapitulino@redhat.com, armbru@redhat.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --D18nW59X0MoGdU6rQa4wVI8OD8AAKKgRe Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 the= re? >>> >>> 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. >=20 > I am afraid that this will miss 2.1 (and so will dataplane=20 > rerror/werror that depends on it), so I went ahead and done all fixes i= n=20 > my qapi-event branch. >=20 > 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). >=20 > Regarding error handling, I have left the code in but switched all > callers to &error_abort. Works for me to use &error_abort. >=20 > The interdiff is here. I ensured that the result is=20 > bisectable, and the intermediate changes are responsible > for the "spurious" hunks of the interdiff: >=20 > 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)/script= s/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 > =20 > -#include > - > 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 *opaq= ue) > * 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... --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --D18nW59X0MoGdU6rQa4wVI8OD8AAKKgRe Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJToGdSAAoJEKeha0olJ0NqNzQIAKDgVw00StlrVpVsVijnBbEn 7YRVD14HVbgtm2FI4BbfJcqbWwx5xHARpUJDGEr9r4Fn+iUDTmMIenrqT4HdnknY xYStyPLrSpPHr9jtMoZHO047kPNG+5ZsOBIUV7Xk9+2m66RHBCdVj7UUZmcdCa3x RO8QxV2nNAqhFlAWljOfObHimyfehowfaACk8U/+J51nDbHaINJQm2LNY8P68pYF P5OxVr0h5yYlyE057q6ClB+ABxAbY1MlX83zdrQHsBt2LyGlu+TAj9BUwViMDIt7 iQd4OonbmP2kdu4ODaIzEjTL4RUICf5W4bXvLqTF36eE8EmdRKwAoPJt1tid9l0= =Z6BE -----END PGP SIGNATURE----- --D18nW59X0MoGdU6rQa4wVI8OD8AAKKgRe--