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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5038 bytes --]

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.

> +++ 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,...

> +    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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]