From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50566) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvyT8-0007TP-Ae for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:38:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvySz-0003ZR-7j for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:38:46 -0400 Received: from mail-pb0-x234.google.com ([2607:f8b0:400e:c01::234]:59230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvySy-0003ZN-SS for qemu-devel@nongnu.org; Sat, 14 Jun 2014 20:38:37 -0400 Received: by mail-pb0-f52.google.com with SMTP id rq2so1200631pbb.39 for ; Sat, 14 Jun 2014 17:38:35 -0700 (PDT) Message-ID: <539CEB04.9030307@gmail.com> Date: Sun, 15 Jun 2014 08:38:28 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1401970944-18735-1-git-send-email-wenchaoqemu@gmail.com> <1401970944-18735-17-git-send-email-wenchaoqemu@gmail.com> <539B6CD3.90905@redhat.com> In-Reply-To: <539B6CD3.90905@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Eric Blake , qemu-devel@nongnu.org Cc: mdroth@linux.vnet.ibm.com, armbru@redhat.com, lcapitulino@redhat.com 于 2014/6/14 5:27, Eric Blake 写道: > 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. > >> >> 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(-) >> > > 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 = NULL; >>> QMPEventFuncEmit emit; >>> QmpOutputVisitor *qov; >>> Visitor *v; >>> QObject *obj; >>> >>> emit = qmp_event_get_func_emit(); >>> if (!emit) { >>> return; >>> } >>> >>> qmp = qmp_event_build_dict("RTC_CHANGE"); >>> >>> qov = qmp_output_visitor_new(); >>> g_assert(qov); >>> >>> v = qmp_output_get_visitor(qov); >>> g_assert(v); >>> >>> /* 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. > >>> >>> visit_type_int(v, &offset, "offset", &local_err); >>> if (local_err) { >>> goto clean; >>> } > > Likewise, qmp_output_type_int never sets errp. > >>> >>> visit_end_struct(v, &local_err); >>> if (local_err) { >>> goto clean; >>> } > > And neither does qmp_end_struct. > >>> >>> obj = qmp_output_get_qobject(qov); >>> g_assert(obj != NULL); >>> >>> 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). > >>> >>> 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, sPAPREnvironment *spapr, >> tm.tm_sec = rtas_ld(args, 5); >> >> /* 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 = 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. > Fixing it. >> +++ b/monitor.c >> @@ -612,6 +612,9 @@ static void monitor_qapi_event_throttle(QAPIEvent event, int64_t rate) >> >> 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,... > Oops, good catch, will fix it. >> + monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000); >> + >> qmp_event_set_func_emit(monitor_qapi_event_queue); >> } >> >> @@ -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); > > ...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 >