From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvZ0w-0006Sj-Ej for qemu-devel@nongnu.org; Fri, 13 Jun 2014 17:28:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvZ0r-0000O4-FI for qemu-devel@nongnu.org; Fri, 13 Jun 2014 17:27:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45123) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvZ0r-0000Ny-6W for qemu-devel@nongnu.org; Fri, 13 Jun 2014 17:27:53 -0400 Message-ID: <539B6CD3.90905@redhat.com> Date: Fri, 13 Jun 2014 15:27:47 -0600 From: Eric Blake MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <1401970944-18735-17-git-send-email-wenchaoqemu@gmail.com> In-Reply-To: <1401970944-18735-17-git-send-email-wenchaoqemu@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Mu6OTnJbe8GGROMNEfef6p3CjARHfrRcO" Subject: Re: [Qemu-devel] [PATCH V6 16/29] qapi event: convert RTC_CHANGE List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Mu6OTnJbe8GGROMNEfef6p3CjARHfrRcO Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 06/05/2014 06:22 AM, Wenchao Xia wrote: > This patch also eliminates build time warning caused by no caller > of monitor_qapi_event_throttle(). Again, my suggestion on 6/29 could avoid that warning; if you use that workaround, don't clean it until 29/29, but you can drop this paragraph of this commit. >=20 > Signed-off-by: Wenchao Xia > --- > docs/qmp/qmp-events.txt | 16 ---------------- > hw/ppc/spapr_rtas.c | 3 ++- > hw/timer/mc146818rtc.c | 3 ++- > include/sysemu/sysemu.h | 2 -- > monitor.c | 4 +++- > qapi-event.json | 13 +++++++++++++ > vl.c | 9 --------- > 7 files changed, 20 insertions(+), 30 deletions(-) >=20 Yay - the first event with data, so I get to see what the generator does.= The .h file shows this signature: >> void qapi_event_send_rtc_change(int64_t offset, >> Error **errp); and the .c has this code: >> void qapi_event_send_rtc_change(int64_t offset, >> Error **errp) >> { >> QDict *qmp; >> Error *local_err =3D NULL; >> QMPEventFuncEmit emit; >> QmpOutputVisitor *qov; >> Visitor *v; >> QObject *obj; >>=20 >> emit =3D qmp_event_get_func_emit(); >> if (!emit) { >> return; >> } >>=20 >> qmp =3D qmp_event_build_dict("RTC_CHANGE"); >>=20 >> qov =3D qmp_output_visitor_new(); >> g_assert(qov); >>=20 >> v =3D qmp_output_get_visitor(qov); >> g_assert(v); >>=20 >> /* Fake visit, as if all member are under a structure */ Grammar error; guess I have more comments on 3/29. >> visit_start_struct(v, NULL, "", "RTC_CHANGE", 0, &local_err); >> if (local_err) { >> goto clean; >> } Hmm, qmp_output_start_struct() never sets errp. >>=20 >> visit_type_int(v, &offset, "offset", &local_err); >> if (local_err) { >> goto clean; >> } Likewise, qmp_output_type_int never sets errp. >>=20 >> visit_end_struct(v, &local_err); >> if (local_err) { >> goto clean; >> } And neither does qmp_end_struct. >>=20 >> obj =3D qmp_output_get_qobject(qov); >> g_assert(obj !=3D NULL); >>=20 >> qdict_put_obj(qmp, "data", obj); >> emit(QAPI_EVENT_RTC_CHANGE, qmp, &local_err); And I already mentioned earlier that the only implementation of the emit() callback never sets local_err (and probably doesn't even need it as a parameter). >>=20 >> clean: >> qmp_output_visitor_cleanup(qov); >> error_propagate(errp, local_err); >> QDECREF(qmp); >> } If you change the earlier 3 instances to just use visit_...(, &error_abort), you can completely ditch the local_err variable, drop the clean: label, and overall have a much shorter generated function. > @@ -93,7 +94,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPA= PREnvironment *spapr, > tm.tm_sec =3D rtas_ld(args, 5); > =20 > /* Just generate a monitor event for the change */ > - rtc_change_mon_event(&tm); > + qapi_event_send_rtc_change(qemu_timedate_diff(&tm), NULL); > spapr->rtc_offset =3D qemu_timedate_diff(&tm); Eww. This makes me worry whether grabbing qemu_timedate_diff() two times in a row may cause a different value to be reported than what is stored locally. Please grab it only once into a local variable, then copy that value into both locations. Once again, all callers of qapi_event_send_rtc_change() are passing a NULL errp to silently ignore errors; and I just audited that no errors happen anyways. > +++ b/monitor.c > @@ -612,6 +612,9 @@ static void monitor_qapi_event_throttle(QAPIEvent e= vent, int64_t rate) > =20 > static void monitor_qapi_event_init(void) > { > + /* Limit RTC & BALLOON events to 1 per second */ Comment is a bit misleading until a later patch converts balloon events,.= =2E. > + monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000); > + > qmp_event_set_func_emit(monitor_qapi_event_queue); > } > =20 > @@ -737,7 +740,6 @@ monitor_protocol_event_throttle(MonitorEvent event,= > static void monitor_protocol_event_init(void) > { > /* Limit RTC & BALLOON events to 1 per second */ > - monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000); > monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000); > monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000); =2E..furthermore, the OLD comment was wrong (it forgot about the watchdog= event). You could get around that by rewording the comment to just say 'limit guest-triggerable events to 1 per second' without calling out what those events are. > +# @RTC_CHANGE > +# > +# Emitted when the guest changes the RTC time. > +# > +# @offset: offset between base RTC clock (as specified by -rtc base), = and > +# new RTC clock value > +# > +# Since: 2.1 0.14.0 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Mu6OTnJbe8GGROMNEfef6p3CjARHfrRcO 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/ iQEcBAEBCAAGBQJTm2zTAAoJEKeha0olJ0NqQwsH/2LPlFIRoGrSHd6WSOGs2GcV cD10u6223tMOYs6XhLkQChQcAf5XjGon25UCed9a4Ah8bBl0owikZQWJMi0JSZko qqt+jsCXG9bQmmyyEHnCsm5IiPjT8yhTvLFaCVK2Mq4KvY7w7sm8BbkQSzGK2jH5 RrHGg855Cb9WHtUdxb75mN8wEesxZmVaTEj8m9/t+8zbuku5iuylkQfm1gYlHqYZ 0EmighazhnkTA0W3ZBPlX0gotBmdULw5xwFj6OsVF17+ieF9SeORHOkuy4yBhkeF X2YKcqMRreMdk3yISevR4HJ8yrudTtnzNigT/iGK/hdN+bghZmsd8tWx88sPy2A= =1wtf -----END PGP SIGNATURE----- --Mu6OTnJbe8GGROMNEfef6p3CjARHfrRcO--