| Message ID | 20221020162558.123284-18-lvivier@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: net: add unix socket type support to netdev backend | expand |
Laurent Vivier <lvivier@redhat.com> writes: > The netdev reports NETDEV_STREAM_CONNECTED event when the backend > is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. Use cases? Could similar event signalling be useful for other kinds of netdev backends? > The NETDEV_STREAM_CONNECTED event includes the URI of the destination > address. No more. Easy fix: scratch "the URI of". > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > net/stream.c | 9 +++++++-- > qapi/net.json | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/net/stream.c b/net/stream.c > index 95d6b910407d..cac01d4d792a 100644 > --- a/net/stream.c > +++ b/net/stream.c > @@ -38,6 +38,7 @@ > #include "io/channel.h" > #include "io/channel-socket.h" > #include "io/net-listener.h" > +#include "qapi/qapi-events-net.h" > > typedef struct NetStreamState { > NetClientState nc; > @@ -168,6 +169,8 @@ static gboolean net_stream_send(QIOChannel *ioc, > s->nc.link_down = true; > qemu_set_info_str(&s->nc, ""); > > + qapi_event_send_netdev_stream_disconnected(s->nc.name); > + > return G_SOURCE_REMOVE; > } > buf = buf1; > @@ -244,8 +247,8 @@ static void net_stream_listen(QIONetListener *listener, > uri = socket_uri(addr); > qemu_set_info_str(&s->nc, uri); > g_free(uri); > + qapi_event_send_netdev_stream_connected(s->nc.name, addr); > qapi_free_SocketAddress(addr); > - Don't add this blank line in PATCH 15, please. > } > > static void net_stream_server_listening(QIOTask *task, gpointer opaque) > @@ -327,7 +330,6 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque) > goto error; > } > g_assert(ret == 0); > - qapi_free_SocketAddress(addr); > > net_socket_rs_init(&s->rs, net_stream_rs_finalize, false); > > @@ -338,6 +340,9 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque) > s, NULL); > s->nc.link_down = false; > > + qapi_event_send_netdev_stream_connected(s->nc.name, addr); > + qapi_free_SocketAddress(addr); > + > return; > error: > object_unref(OBJECT(s->ioc)); Could put the qapi_free_SocketAddress() in its final place in PATCH 15 already to reduce churn. Up to you. > diff --git a/qapi/net.json b/qapi/net.json > index 39388b1b6c41..c37b24717382 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -895,3 +895,52 @@ > ## > { 'event': 'FAILOVER_NEGOTIATED', > 'data': {'device-id': 'str'} } > + > +## > +# @NETDEV_STREAM_CONNECTED: > +# > +# Emitted when the netdev stream backend is connected > +# > +# @netdev-id: QEMU netdev id that is connected > +# @addr: The destination address > +# > +# Since: 7.2 > +# > +# Example: > +# > +# <- { "event": "NETDEV_STREAM_CONNECTED", > +# "data": { "netdev-id": "netdev0", > +# "addr": { "port": "47666", "ipv6": true, > +# "host": "::1", "type": "inet" } }, > +# "timestamp": { "seconds": 1666269863, "microseconds": 311222 } } > +# > +# or > +# > +# <- { "event": "NETDEV_STREAM_CONNECTED", > +# "data": { "netdev-id": "netdev0", > +# "addr": { "path": "/tmp/qemu0", "type": "unix" } }, > +# "timestamp": { "seconds": 1666269706, "microseconds": 413651 } } > +# > +## > +{ 'event': 'NETDEV_STREAM_CONNECTED', > + 'data': { 'netdev-id': 'str', > + 'addr': 'SocketAddress' } } > + > +## > +# @NETDEV_STREAM_DISCONNECTED: > +# > +# Emitted when the netdev stream backend is disconnected > +# > +# @netdev-id: QEMU netdev id that is disconnected > +# > +# Since: 7.2 > +# > +# Example: > +# > +# <- { 'event': 'NETDEV_STREAM_DISCONNECTED', > +# 'data': {'netdev-id': 'netdev0'}, > +# 'timestamp': {'seconds': 1663330937, 'microseconds': 526695} } > +# > +## > +{ 'event': 'NETDEV_STREAM_DISCONNECTED', > + 'data': { 'netdev-id': 'str' } } Schema looks good to me.
On 10/21/22 07:48, Markus Armbruster wrote: > Laurent Vivier <lvivier@redhat.com> writes: > >> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. > > Use cases? This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. I have also a patch to add a "reconnect=seconds" option, but I didn't want to add it to this series. > > Could similar event signalling be useful for other kinds of netdev > backends? I was wondering, but it becomes more complicated to be generic. Thanks, Laurent
Cc: Stefano Brivio Laurent Vivier <lvivier@redhat.com> writes: > On 10/21/22 07:48, Markus Armbruster wrote: >> Laurent Vivier <lvivier@redhat.com> writes: >> >>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >> >> Use cases? > > This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. Let's add something like this to the commit message: This lets libvirt notice when the connection is lost somehow, and restart the peer (such as passt). Who's working on the libvirt part? > I have also a patch to add a "reconnect=seconds" option, but I didn't want to add it to this series. It's okay to mention future work in commit messages, but not required. >> Could similar event signalling be useful for other kinds of netdev >> backends? > > I was wondering, but it becomes more complicated to be generic. Making something complicated and generic where a simpler special solution would do is the worst. Not quite as bad (but still plenty bad) is making a few special solutions first, then replace them all with a generic solution. I believe we should have a good, hard think on possible applications of a generic solution now. There is no need to hold back this series for that. If we conclude a generic solution is called for, we better replace this special solution before it becomes ABI. Either by replacing it before we release it, or by keeping it unstable until we replace it.
On 10/21/22 11:12, Markus Armbruster wrote: > Cc: Stefano Brivio > > Laurent Vivier <lvivier@redhat.com> writes: > >> On 10/21/22 07:48, Markus Armbruster wrote: >>> Laurent Vivier <lvivier@redhat.com> writes: >>> >>>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >>>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >>> >>> Use cases? >> >> This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. > > Let's add something like this to the commit message: > > This lets libvirt notice when the connection is lost somehow, and > restart the peer (such as passt). > > Who's working on the libvirt part? > >> I have also a patch to add a "reconnect=seconds" option, but I didn't want to add it to this series. > > It's okay to mention future work in commit messages, but not required. > >>> Could similar event signalling be useful for other kinds of netdev >>> backends? >> >> I was wondering, but it becomes more complicated to be generic. > > Making something complicated and generic where a simpler special > solution would do is the worst. > > Not quite as bad (but still plenty bad) is making a few special > solutions first, then replace them all with a generic solution. > > I believe we should have a good, hard think on possible applications of > a generic solution now. > > There is no need to hold back this series for that. > > If we conclude a generic solution is called for, we better replace this > special solution before it becomes ABI. Either by replacing it before > we release it, or by keeping it unstable until we replace it. > I sent the v14 few minutes before this email. Jason, perhaps we can remove PATCH 17 from the series and only merge PATCH 1 to 16? I will resend PATCH 17 in a new series with the reconnect option patch once this series is merged. Thanks, Laurent
[Cc: Laine, full quote] On Fri, 21 Oct 2022 11:12:20 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Cc: Stefano Brivio > > Laurent Vivier <lvivier@redhat.com> writes: > > > On 10/21/22 07:48, Markus Armbruster wrote: > >> Laurent Vivier <lvivier@redhat.com> writes: > >> > >>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend > >>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. > >> > >> Use cases? > > > > This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. > > Let's add something like this to the commit message: > > This lets libvirt notice when the connection is lost somehow, and > restart the peer (such as passt). > > Who's working on the libvirt part? Laine Stump and myself. Nothing to show yet, though. > > I have also a patch to add a "reconnect=seconds" option, but I didn't want to add it to this series. > > It's okay to mention future work in commit messages, but not required. > > >> Could similar event signalling be useful for other kinds of netdev > >> backends? > > > > I was wondering, but it becomes more complicated to be generic. > > Making something complicated and generic where a simpler special > solution would do is the worst. > > Not quite as bad (but still plenty bad) is making a few special > solutions first, then replace them all with a generic solution. > > I believe we should have a good, hard think on possible applications of > a generic solution now. > > There is no need to hold back this series for that. > > If we conclude a generic solution is called for, we better replace this > special solution before it becomes ABI. Either by replacing it before > we release it, or by keeping it unstable until we replace it.
Laurent Vivier <lvivier@redhat.com> writes: > On 10/21/22 11:12, Markus Armbruster wrote: >> Cc: Stefano Brivio >> >> Laurent Vivier <lvivier@redhat.com> writes: >> >>> On 10/21/22 07:48, Markus Armbruster wrote: >>>> Laurent Vivier <lvivier@redhat.com> writes: >>>> >>>>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >>>>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >>>> >>>> Use cases? >>> >>> This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. >> >> Let's add something like this to the commit message: >> >> This lets libvirt notice when the connection is lost somehow, and >> restart the peer (such as passt). >> >> Who's working on the libvirt part? >> >>> I have also a patch to add a "reconnect=seconds" option, but I didn't want to add it to this series. >> >> It's okay to mention future work in commit messages, but not required. >> >>>> Could similar event signalling be useful for other kinds of netdev >>>> backends? >>> >>> I was wondering, but it becomes more complicated to be generic. >> >> Making something complicated and generic where a simpler special >> solution would do is the worst. >> >> Not quite as bad (but still plenty bad) is making a few special >> solutions first, then replace them all with a generic solution. >> >> I believe we should have a good, hard think on possible applications of >> a generic solution now. >> >> There is no need to hold back this series for that. >> >> If we conclude a generic solution is called for, we better replace this >> special solution before it becomes ABI. Either by replacing it before >> we release it, or by keeping it unstable until we replace it. >> > > I sent the v14 few minutes before this email. > > Jason, perhaps we can remove PATCH 17 from the series and only merge PATCH 1 to 16? > > I will resend PATCH 17 in a new series with the reconnect option patch once this series is > merged. Certainly works for me. Thanks for your patience!
Stefano Brivio <sbrivio@redhat.com> writes: > [Cc: Laine, full quote] > > On Fri, 21 Oct 2022 11:12:20 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Cc: Stefano Brivio >> >> Laurent Vivier <lvivier@redhat.com> writes: >> >> > On 10/21/22 07:48, Markus Armbruster wrote: >> >> Laurent Vivier <lvivier@redhat.com> writes: >> >> >> >>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >> >>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >> >> >> >> Use cases? >> > >> > This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. >> >> Let's add something like this to the commit message: >> >> This lets libvirt notice when the connection is lost somehow, and >> restart the peer (such as passt). >> >> Who's working on the libvirt part? > > Laine Stump and myself. Nothing to show yet, though. Good enough for me :) [...]
Markus Armbruster <armbru@redhat.com> writes: > Cc: Stefano Brivio > > Laurent Vivier <lvivier@redhat.com> writes: > >> On 10/21/22 07:48, Markus Armbruster wrote: >>> Laurent Vivier <lvivier@redhat.com> writes: >>> >>>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >>>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >>> >>> Use cases? >> >> This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. [...] >>> Could similar event signalling be useful for other kinds of netdev >>> backends? >> >> I was wondering, but it becomes more complicated to be generic. > > Making something complicated and generic where a simpler special > solution would do is the worst. > > Not quite as bad (but still plenty bad) is making a few special > solutions first, then replace them all with a generic solution. > > I believe we should have a good, hard think on possible applications of > a generic solution now. > > There is no need to hold back this series for that. > > If we conclude a generic solution is called for, we better replace this > special solution before it becomes ABI. Either by replacing it before > we release it, or by keeping it unstable until we replace it. Stefano, any thoughts on this?
On Mon, 24 Oct 2022 13:00:09 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Markus Armbruster <armbru@redhat.com> writes: > > > Cc: Stefano Brivio > > > > Laurent Vivier <lvivier@redhat.com> writes: > > > >> On 10/21/22 07:48, Markus Armbruster wrote: > >>> Laurent Vivier <lvivier@redhat.com> writes: > >>> > >>>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend > >>>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. > >>> > >>> Use cases? > >> > >> This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. > > [...] > > >>> Could similar event signalling be useful for other kinds of netdev > >>> backends? > >> > >> I was wondering, but it becomes more complicated to be generic. > > > > Making something complicated and generic where a simpler special > > solution would do is the worst. > > > > Not quite as bad (but still plenty bad) is making a few special > > solutions first, then replace them all with a generic solution. > > > > I believe we should have a good, hard think on possible applications of > > a generic solution now. > > > > There is no need to hold back this series for that. > > > > If we conclude a generic solution is called for, we better replace this > > special solution before it becomes ABI. Either by replacing it before > > we release it, or by keeping it unstable until we replace it. > > Stefano, any thoughts on this? Actually, to me, it already looks as generic as it can be: stream back-ends are the only ones connecting and disconnecting. I quickly tried to think about possible, similar events for other back-ends: - user: handled by libslirp, there's no connection, and probably not much we can or want to export from libslirp itself - tap, bridge: the closest equivalent would be interfaces changing states, but that's something that's also externally observable with a netlink socket, in case one needs to know. And in any case, it's logically very different from a connection or disconnection. If we want events for that, they should have different names - vhost-user, vde: we could implement something similar if the need arises, but it should logically have a different name - l2tpv3: stateless, same as datagram-oriented socket. No states, no events to report, I guess. All in all, to me, NETDEV_STREAM_{,DIS}CONNECTED events here don't look very "special" or hackish.
Stefano Brivio <sbrivio@redhat.com> writes: > On Mon, 24 Oct 2022 13:00:09 +0200 > Markus Armbruster <armbru@redhat.com> wrote: > >> Markus Armbruster <armbru@redhat.com> writes: >> >> > Cc: Stefano Brivio >> > >> > Laurent Vivier <lvivier@redhat.com> writes: >> > >> >> On 10/21/22 07:48, Markus Armbruster wrote: >> >>> Laurent Vivier <lvivier@redhat.com> writes: >> >>> >> >>>> The netdev reports NETDEV_STREAM_CONNECTED event when the backend >> >>>> is connected, and NETDEV_STREAM_DISCONNECTED when it is disconnected. >> >>> >> >>> Use cases? >> >> >> >> This is asked by Stefano Brivio to allow libvirt to detect if connection to passt is lost and to restart passt. >> >> [...] >> >> >>> Could similar event signalling be useful for other kinds of netdev >> >>> backends? >> >> >> >> I was wondering, but it becomes more complicated to be generic. >> > >> > Making something complicated and generic where a simpler special >> > solution would do is the worst. >> > >> > Not quite as bad (but still plenty bad) is making a few special >> > solutions first, then replace them all with a generic solution. >> > >> > I believe we should have a good, hard think on possible applications of >> > a generic solution now. >> > >> > There is no need to hold back this series for that. >> > >> > If we conclude a generic solution is called for, we better replace this >> > special solution before it becomes ABI. Either by replacing it before >> > we release it, or by keeping it unstable until we replace it. >> >> Stefano, any thoughts on this? > > Actually, to me, it already looks as generic as it can be: stream > back-ends are the only ones connecting and disconnecting. > > I quickly tried to think about possible, similar events for other > back-ends: > > - user: handled by libslirp, there's no connection, and probably not > much we can or want to export from libslirp itself > > - tap, bridge: the closest equivalent would be interfaces changing > states, but that's something that's also externally observable with a > netlink socket, in case one needs to know. And in any case, it's > logically very different from a connection or disconnection. If we > want events for that, they should have different names > > - vhost-user, vde: we could implement something similar if the need > arises, but it should logically have a different name > > - l2tpv3: stateless, same as datagram-oriented socket. No states, no > events to report, I guess. > > All in all, to me, NETDEV_STREAM_{,DIS}CONNECTED events here don't look > very "special" or hackish. Thanks!
diff --git a/net/stream.c b/net/stream.c index 95d6b910407d..cac01d4d792a 100644 --- a/net/stream.c +++ b/net/stream.c @@ -38,6 +38,7 @@ #include "io/channel.h" #include "io/channel-socket.h" #include "io/net-listener.h" +#include "qapi/qapi-events-net.h" typedef struct NetStreamState { NetClientState nc; @@ -168,6 +169,8 @@ static gboolean net_stream_send(QIOChannel *ioc, s->nc.link_down = true; qemu_set_info_str(&s->nc, ""); + qapi_event_send_netdev_stream_disconnected(s->nc.name); + return G_SOURCE_REMOVE; } buf = buf1; @@ -244,8 +247,8 @@ static void net_stream_listen(QIONetListener *listener, uri = socket_uri(addr); qemu_set_info_str(&s->nc, uri); g_free(uri); + qapi_event_send_netdev_stream_connected(s->nc.name, addr); qapi_free_SocketAddress(addr); - } static void net_stream_server_listening(QIOTask *task, gpointer opaque) @@ -327,7 +330,6 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque) goto error; } g_assert(ret == 0); - qapi_free_SocketAddress(addr); net_socket_rs_init(&s->rs, net_stream_rs_finalize, false); @@ -338,6 +340,9 @@ static void net_stream_client_connected(QIOTask *task, gpointer opaque) s, NULL); s->nc.link_down = false; + qapi_event_send_netdev_stream_connected(s->nc.name, addr); + qapi_free_SocketAddress(addr); + return; error: object_unref(OBJECT(s->ioc)); diff --git a/qapi/net.json b/qapi/net.json index 39388b1b6c41..c37b24717382 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -895,3 +895,52 @@ ## { 'event': 'FAILOVER_NEGOTIATED', 'data': {'device-id': 'str'} } + +## +# @NETDEV_STREAM_CONNECTED: +# +# Emitted when the netdev stream backend is connected +# +# @netdev-id: QEMU netdev id that is connected +# @addr: The destination address +# +# Since: 7.2 +# +# Example: +# +# <- { "event": "NETDEV_STREAM_CONNECTED", +# "data": { "netdev-id": "netdev0", +# "addr": { "port": "47666", "ipv6": true, +# "host": "::1", "type": "inet" } }, +# "timestamp": { "seconds": 1666269863, "microseconds": 311222 } } +# +# or +# +# <- { "event": "NETDEV_STREAM_CONNECTED", +# "data": { "netdev-id": "netdev0", +# "addr": { "path": "/tmp/qemu0", "type": "unix" } }, +# "timestamp": { "seconds": 1666269706, "microseconds": 413651 } } +# +## +{ 'event': 'NETDEV_STREAM_CONNECTED', + 'data': { 'netdev-id': 'str', + 'addr': 'SocketAddress' } } + +## +# @NETDEV_STREAM_DISCONNECTED: +# +# Emitted when the netdev stream backend is disconnected +# +# @netdev-id: QEMU netdev id that is disconnected +# +# Since: 7.2 +# +# Example: +# +# <- { 'event': 'NETDEV_STREAM_DISCONNECTED', +# 'data': {'netdev-id': 'netdev0'}, +# 'timestamp': {'seconds': 1663330937, 'microseconds': 526695} } +# +## +{ 'event': 'NETDEV_STREAM_DISCONNECTED', + 'data': { 'netdev-id': 'str' } }