qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V6 16/29] qapi event: convert RTC_CHANGE


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V6 16/29] qapi event: convert RTC_CHANGE
Date: Sun, 15 Jun 2014 08:38:28 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

于 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 <address@hidden>
---
  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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]