Re: [PATCH v4] net: vhost-user: add QAPI events to report connection state

[Laurent Vivier <lvivier@redhat.com>]

On 18/02/2025 08:50, Markus Armbruster wrote:
> Laurent Vivier <lvivier@redhat.com> writes:
> 
>> The netdev reports NETDEV_VHOST_USER_CONNECTED event when
>> the chardev is connected, and NETDEV_VHOST_USER_DISCONNECTED
>> when it is disconnected.
>>
>> The NETDEV_VHOST_USER_CONNECTED event includes the chardev id.
>>
>> This allows a system manager like libvirt to detect when the server
>> fails.
>>
>> For instance with passt:
>>
>> { 'execute': 'qmp_capabilities' }
>> { "return": { } }
>>
>> [killing passt here]
>>
>> { "timestamp": { "seconds": 1739538634, "microseconds": 920450 },
>>    "event": "NETDEV_VHOST_USER_DISCONNECTED",
>>    "data": { "netdev-id": "netdev0" } }
>>
>> [automatic reconnection with reconnect-ms]
>>
>> { "timestamp": { "seconds": 1739538638, "microseconds": 354181 },
>>    "event": "NETDEV_VHOST_USER_CONNECTED",
>>    "data": { "netdev-id": "netdev0", "chardev-id": "chr0" } }
>>
>> Tested-by: Stefano Brivio <sbrivio@redhat.com>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>
>> Notes:
>>      v4:
>>        - as ChardevInfo is not needed, move events definition from
>>          qapi/char.json to qapi/net.json
>>      
>>      v3:
>>        - remove ChardevInfo, provides only the chardev id
>>      
>>      v2:
>>        - remove duplicate line info.frontend_open
>>
>>   qapi/net.json    | 40 ++++++++++++++++++++++++++++++++++++++++
>>   net/vhost-user.c |  3 +++
>>   2 files changed, 43 insertions(+)
>>
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 2739a2f42332..310cc4fd1907 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -1031,3 +1031,43 @@
>>   ##
>>   { 'event': 'NETDEV_STREAM_DISCONNECTED',
>>     'data': { 'netdev-id': 'str' } }
>> +
>> +##
>> +# @NETDEV_VHOST_USER_CONNECTED:
>> +#
>> +# Emitted when the vhost-user chardev is connected
>> +#
>> +# @netdev-id: QEMU netdev id that is connected
>> +#
>> +# @chardev-id: The character device id used by the QEMU netdev
>> +#
>> +# Since: 10.0
>> +#
>> +# .. qmp-example::
>> +#
>> +#     <- { "timestamp": {"seconds": 1739538638, "microseconds": 354181 },
>> +#          "event": "NETDEV_VHOST_USER_CONNECTED",
>> +#          "data": { "netdev-id": "netdev0", "chardev-id": "chr0" } }
>> +#
>> +##
>> +{ 'event': 'NETDEV_VHOST_USER_CONNECTED',
>> +  'data': { 'netdev-id': 'str', 'chardev-id': 'str' } }
>> +
>> +##
>> +# @NETDEV_VHOST_USER_DISCONNECTED:
>> +#
>> +# Emitted when the vhost-user chardev is disconnected
>> +#
>> +# @netdev-id: QEMU netdev id that is disconnected
>> +#
>> +# Since: 10.0
>> +#
>> +# .. qmp-example::
>> +#
>> +#     <- { "timestamp": { "seconds": 1739538634, "microseconds": 920450 },
>> +#          "event": "NETDEV_VHOST_USER_DISCONNECTED",
>> +#          "data": { "netdev-id": "netdev0" } }
>> +#
>> +##
>> +{ 'event': 'NETDEV_VHOST_USER_DISCONNECTED',
>> +  'data': { 'netdev-id': 'str' } }
>> diff --git a/net/vhost-user.c b/net/vhost-user.c
>> index 12555518e838..0b235e50c650 100644
>> --- a/net/vhost-user.c
>> +++ b/net/vhost-user.c
>> @@ -16,6 +16,7 @@
>>   #include "chardev/char-fe.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-net.h"
>> +#include "qapi/qapi-events-net.h"
>>   #include "qemu/config-file.h"
>>   #include "qemu/error-report.h"
>>   #include "qemu/option.h"
>> @@ -271,6 +272,7 @@ static void chr_closed_bh(void *opaque)
>>       if (err) {
>>           error_report_err(err);
>>       }
>> +    qapi_event_send_netdev_vhost_user_disconnected(name);
>>   }
>>   
>>   static void net_vhost_user_event(void *opaque, QEMUChrEvent event)
>> @@ -300,6 +302,7 @@ static void net_vhost_user_event(void *opaque, QEMUChrEvent event)
>>                                            net_vhost_user_watch, s);
>>           qmp_set_link(name, true, &err);
>>           s->started = true;
>> +        qapi_event_send_netdev_vhost_user_connected(name, chr->label);
> 
> We seem to use "label" and "id" interchangeably.  Unfortunate.
> 
> 
>>           break;
>>       case CHR_EVENT_CLOSED:
>>           /* a close event may happen during a read/write, but vhost
> 
> Like Daniel, I wonder whether provding events for chardevs instead would
> be more broadly useful.

In fact, it depends on what libvirt needs or wants.

If we implement the event for chardev, libvirt will have to find internally which netdev 
it belongs to, if we implement the event for netdev it has directly the netdev id.

But no one never asked for a chardev event until now, not sure it's useful.

Stefano shown that vhost-user event is really easy to use with existing code of libvirt.

> 
> That said, there's nothing wrong with the patch itself, so
> Acked-by: Markus Armbruster <armbru@redhat.com>
> 

Thanks,
Laurent