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

[Wenchao Xia <wenchaoqemu@gmail.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 <wenchaoqemu@gmail.com>
>> ---
>>   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
>