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

2014-06-17 Thread Paolo Bonzini 

Il 13/06/2014 23:27, Eric Blake ha scritto:

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


0.13.0 :)

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

2014-06-17 Thread Eric Blake 
On 06/17/2014 03:21 AM, Paolo Bonzini wrote:
 Il 13/06/2014 23:27, Eric Blake ha scritto:
  +# @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
 
 0.13.0 :)

Again, since nothing else in QAPI documents anything earlier than 0.14,
I'm not sure it's worth caring about whether 0.13 supported a particular
event.

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



signature.asc
Description: OpenPGP digital signature

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

2014-06-15 Thread Paolo Bonzini 

Il 13/06/2014 23:27, Eric Blake ha scritto:

 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.



I think it is better to produce correct error propagation even if it is 
unused.  We could add range-checking of enums, for example.


I guess all the NULLs for errp could become error_abort, but it can be 
done after the merge.


Paolo

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

2014-06-15 Thread Paolo Bonzini 

Il 15/06/2014 02:38, Wenchao Xia ha scritto:


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.


No, please don't.  I prefer the way you did it in v6.

Paolo

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

2014-06-14 Thread Wenchao Xia 

于 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 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.



  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

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

2014-06-13 Thread 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 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