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 <wenchaoq...@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
signature.asc
Description: OpenPGP digital signature
