qemu-arm
[Top][All Lists]
Advanced

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

Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event


From: Markus Armbruster
Subject: Re: [RFC PATCH 2/5] hw/misc/led: Add LED_STATUS_CHANGED QAPI event
Date: Mon, 15 Jun 2020 07:59:50 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Eric,
>
> On 6/9/20 4:29 PM, Eric Blake wrote:
>> On 6/9/20 7:34 AM, Philippe Mathieu-Daudé wrote:
>>> Allow LED devices to emit STATUS_CHANGED events on a QMP chardev.
>>>
>>> QMP event examples:
>>>
>>> {
>>>      "timestamp": {
>>>          "seconds": 1591704274,
>>>          "microseconds": 520850
>>>      },
>>>      "event": "LED_STATUS_CHANGED",
>>>      "data": {
>>>          "name": "Green LED #0",
>>>          "status": "on"
>>>      }
>>> }
>>> {
>>>      "timestamp": {
>>>          "seconds": 1591704275,
>>>          "microseconds": 530912
>>>      },
>>>      "event": "LED_STATUS_CHANGED",
>>>      "data": {
>>>          "name": "Green LED #0",
>>>          "status": "off"
>>>      }
>>> }
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>> 
>> The QAPI addition looks reasonable, however,
>> 
>>> +++ b/hw/misc/led.c
>>> @@ -7,6 +7,7 @@
>>>    */
>>>   #include "qemu/osdep.h"
>>>   #include "qapi/error.h"
>>> +#include "qapi/qapi-events-led.h"
>>>   #include "hw/qdev-properties.h"
>>>   #include "hw/misc/led.h"
>>>   #include "hw/irq.h"
>>> @@ -19,6 +20,9 @@ static void led_set(void *opaque, int line, int
>>> new_state)
>>>         trace_led_set(s->name, s->current_state, new_state);
>>>   +    /* FIXME QMP rate limite? */
>> 
>> s/limite/limit/
>> 
>> Yes, this is under guest control, so you MUST rate limit to avoid the
>> guest being able to DoS qemu by changing the LED so frequently as to
>> overwhelm the QMP connection with events.
>
> Commits f544d174dfc and 7f1e7b23d5 refers to the qmp-events.txt
> for documentation on rate-limiting QMP events, but I can't find
> it in the codebase. Two files matches 'qmp-events' but don't have
> documentation: qapi/qmp-event.c and include/qapi/qmp-event.h.
>
> Last trace of it is in commit 231aaf3a8217. Apparently it was
> somehow split qapi/event.json, then later c09656f1d392 move it
> to qapi-schema.json, finally eb815e248f50 moved it to qapi/.
>
> Is the referred documentation now in docs/devel/qapi-code-gen.txt?
> There is only one occurence of 'limit' but it is unrelated to
> rate-limit.

Commit 231aaf3a8217 is part of Marc-André's herculean QAPI/QMP doc
reorganization: use only schema doc comments instead of spreading the
knowledge over schema and several other files, with duplicated contents,
confused readers, and annoyed writers.

Before the reorganization, docs/qmp-events.txt listed the QMP events,
and rate-limited events carried a

    Note: this event is rate-limited.

The reorganization moved this note into its event's doc comment.
Example:

    ##
    # @WATCHDOG:
    #
    # Emitted when the watchdog device's timer is expired
    #
    # @action: action that has been taken
    #
    # Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
    #       followed respectively by the RESET, SHUTDOWN, or STOP events
    #
--> # Note: This event is rate-limited.
    #
    [...]
    ##
    { 'event': 'WATCHDOG',
      'data': { 'action': 'WatchdogAction' } }

The QMP *protocol* is still documented in docs/interop/qmp-spec.txt.
Relevant part:

    2.5 Asynchronous events
    -----------------------

    As a result of state changes, the Server may send messages unilaterally
    to the Client at any time, when not in the middle of any other
    response. They are called "asynchronous events".

    The format of asynchronous events is:

    { "event": json-string, "data": json-object,
      "timestamp": { "seconds": json-number, "microseconds": json-number } }

     Where,

    - The "event" member contains the event's name
    - The "data" member contains event specific data, which is defined in a
      per-event basis, it is optional
    - The "timestamp" member contains the exact time of when the event
      occurred in the Server. It is a fixed json-object with time in
      seconds and microseconds relative to the Unix Epoch (1 Jan 1970); if
      there is a failure to retrieve host time, both members of the
      timestamp will be set to -1.

    For a listing of supported asynchronous events, please, refer to the
    qmp-events.txt file.

    Some events are rate-limited to at most one per second.  If additional
    "similar" events arrive within one second, all but the last one are
    dropped, and the last one is delayed.  "Similar" normally means same
    event type.  See qmp-events.txt for details.

The reorganization neglected to update it for the removal of
qmp-events.txt.  Should point to
docs/interop/qemu-qmp-ref.{7,html,info,pdf,txt} now.

Event rate-limiting is defined in monitor_qapi_event_conf[].  To
rate-limit an event, add it to monitor_qapi_event_conf[], and also add
the "Note: This event is rate-limited." to its schema doc comment.

Doc bug: commit e2ae6159de "virtio-serial: report frontend connection
state via monitor" neglected to add the note.

Patches welcome!




reply via email to

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