Re: [PATCH v14 15/17] net: stream: move to QIO to enable additional parameters

2022-10-21 Thread Philippe Mathieu-Daudé 

On 21/10/22 13:31, Markus Armbruster wrote:

Laurent Vivier  writes:


On 10/21/22 12:35, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


On 21/10/22 11:09, Laurent Vivier wrote:

Use QIOChannel, QIOChannelSocket and QIONetListener.
This allows net/stream to use all the available parameters provided by
SocketAddress.
Signed-off-by: Laurent Vivier 
Acked-by: Michael S. Tsirkin 
---
net/stream.c| 492 +---
qemu-options.hx |   4 +-
2 files changed, 178 insertions(+), 318 deletions(-)



+addr = qio_channel_socket_get_local_address(listen_sioc, NULL);
+g_assert(addr != NULL);


Missing propagating Error* (observed in v12).


*If* this is really a programming error: what about _abort?


assert() informs the compiler that following code will not use addr with a NULL 
value, I
don't think _abort does that. This could avoid an error report in code 
static analyzer.


I'd expect Coverity to see right through it.

Static analyzers with a less global view won't, of course.

For what it's worth, there are about a thousand uses of _abort
outside tests/.  I'm not aware of them confusing static analyzers we
care about.

I like _abort, because it makes the program crash when we try to
put the error into _abort, with an informative message.  This is
often right where things go wrong[*].  I personally don't care much
about the better message, but others do.  The better stack backtrace has
been quite useful to me.


I concur:

  qemu-system-x86_64: socket family 0 unsupported

VS:

   ERROR:../../net/stream.c:321:net_stream_client_connected: assertion
failed: (addr != NULL)

https://lore.kernel.org/qemu-devel/6fa6b9e5-fede-0f68-752f-0c0d8fa34...@linaro.org/



Let's use _abort, and throw in the assert when a static analyzer
we care about needs it.


[*] error_propagate() messes this up.  That's why the comments in
error.h ask you to do without when practical.

Re: [PATCH v14 15/17] net: stream: move to QIO to enable additional parameters

2022-10-21 Thread Markus Armbruster 
Laurent Vivier  writes:

> On 10/21/22 12:35, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé  writes:
>> 
>>> On 21/10/22 11:09, Laurent Vivier wrote:
 Use QIOChannel, QIOChannelSocket and QIONetListener.
 This allows net/stream to use all the available parameters provided by
 SocketAddress.
 Signed-off-by: Laurent Vivier 
 Acked-by: Michael S. Tsirkin 
 ---
net/stream.c| 492 +---
qemu-options.hx |   4 +-
2 files changed, 178 insertions(+), 318 deletions(-)
>>>
 -static void net_stream_accept(void *opaque)
 +static void net_stream_server_listening(QIOTask *task, gpointer opaque)
{
NetStreamState *s = opaque;
 -struct sockaddr_storage saddr;
 -socklen_t len;
 -int fd;
 -
 -for (;;) {
 -len = sizeof(saddr);
 -fd = qemu_accept(s->listen_fd, (struct sockaddr *), );
 -if (fd < 0 && errno != EINTR) {
 -return;
 -} else if (fd >= 0) {
 -qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
 -break;
 -}
 -}
 +QIOChannelSocket *listen_sioc = QIO_CHANNEL_SOCKET(s->listen_ioc);
 +SocketAddress *addr;
 +int ret;
 -s->fd = fd;
 -s->nc.link_down = false;
 -net_stream_connect(s);
 -switch (saddr.ss_family) {
 -case AF_INET: {
 -struct sockaddr_in *saddr_in = (struct sockaddr_in *)
 -
 -qemu_set_info_str(>nc, "connection from %s:%d",
 -  inet_ntoa(saddr_in->sin_addr),
 -  ntohs(saddr_in->sin_port));
 -break;
 +if (listen_sioc->fd < 0) {
 +qemu_set_info_str(>nc, "connection error");
 +return;
}
 -case AF_UNIX: {
 -struct sockaddr_un saddr_un;
 -len = sizeof(saddr_un);
 -getsockname(s->listen_fd, (struct sockaddr *)_un, );
 -qemu_set_info_str(>nc, "connect from %s", saddr_un.sun_path);
 -break;
 -}
 -default:
 -g_assert_not_reached();
 +addr = qio_channel_socket_get_local_address(listen_sioc, NULL);
 +g_assert(addr != NULL);
>>>
>>> Missing propagating Error* (observed in v12).
>> 
>> *If* this is really a programming error: what about _abort?
>
> assert() informs the compiler that following code will not use addr with a 
> NULL value, I 
> don't think _abort does that. This could avoid an error report in code 
> static analyzer.

I'd expect Coverity to see right through it.

Static analyzers with a less global view won't, of course.

For what it's worth, there are about a thousand uses of _abort
outside tests/.  I'm not aware of them confusing static analyzers we
care about.

I like _abort, because it makes the program crash when we try to
put the error into _abort, with an informative message.  This is
often right where things go wrong[*].  I personally don't care much
about the better message, but others do.  The better stack backtrace has
been quite useful to me.

Let's use _abort, and throw in the assert when a static analyzer
we care about needs it.


[*] error_propagate() messes this up.  That's why the comments in
error.h ask you to do without when practical.

Re: [PATCH v14 15/17] net: stream: move to QIO to enable additional parameters

2022-10-21 Thread Laurent Vivier 

On 10/21/22 12:35, Markus Armbruster wrote:

Philippe Mathieu-Daudé  writes:


On 21/10/22 11:09, Laurent Vivier wrote:

Use QIOChannel, QIOChannelSocket and QIONetListener.
This allows net/stream to use all the available parameters provided by
SocketAddress.
Signed-off-by: Laurent Vivier 
Acked-by: Michael S. Tsirkin 
---
   net/stream.c| 492 +---
   qemu-options.hx |   4 +-
   2 files changed, 178 insertions(+), 318 deletions(-)



-static void net_stream_accept(void *opaque)
+static void net_stream_server_listening(QIOTask *task, gpointer opaque)
   {
   NetStreamState *s = opaque;
-struct sockaddr_storage saddr;
-socklen_t len;
-int fd;
-
-for (;;) {
-len = sizeof(saddr);
-fd = qemu_accept(s->listen_fd, (struct sockaddr *), );
-if (fd < 0 && errno != EINTR) {
-return;
-} else if (fd >= 0) {
-qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
-break;
-}
-}
+QIOChannelSocket *listen_sioc = QIO_CHANNEL_SOCKET(s->listen_ioc);
+SocketAddress *addr;
+int ret;
   -s->fd = fd;
-s->nc.link_down = false;
-net_stream_connect(s);
-switch (saddr.ss_family) {
-case AF_INET: {
-struct sockaddr_in *saddr_in = (struct sockaddr_in *)
-
-qemu_set_info_str(>nc, "connection from %s:%d",
-  inet_ntoa(saddr_in->sin_addr),
-  ntohs(saddr_in->sin_port));
-break;
+if (listen_sioc->fd < 0) {
+qemu_set_info_str(>nc, "connection error");
+return;
   }
-case AF_UNIX: {
-struct sockaddr_un saddr_un;
   -len = sizeof(saddr_un);
-getsockname(s->listen_fd, (struct sockaddr *)_un, );
-qemu_set_info_str(>nc, "connect from %s", saddr_un.sun_path);
-break;
-}
-default:
-g_assert_not_reached();
+addr = qio_channel_socket_get_local_address(listen_sioc, NULL);
+g_assert(addr != NULL);


Missing propagating Error* (observed in v12).


*If* this is really a programming error: what about _abort?


assert() informs the compiler that following code will not use addr with a NULL value, I 
don't think _abort does that. This could avoid an error report in code static analyzer.


Thanks,
Laurent

Re: [PATCH v14 15/17] net: stream: move to QIO to enable additional parameters

2022-10-21 Thread Markus Armbruster 
Philippe Mathieu-Daudé  writes:

> On 21/10/22 11:09, Laurent Vivier wrote:
>> Use QIOChannel, QIOChannelSocket and QIONetListener.
>> This allows net/stream to use all the available parameters provided by
>> SocketAddress.
>> Signed-off-by: Laurent Vivier 
>> Acked-by: Michael S. Tsirkin 
>> ---
>>   net/stream.c| 492 +---
>>   qemu-options.hx |   4 +-
>>   2 files changed, 178 insertions(+), 318 deletions(-)
>
>> -static void net_stream_accept(void *opaque)
>> +static void net_stream_server_listening(QIOTask *task, gpointer opaque)
>>   {
>>   NetStreamState *s = opaque;
>> -struct sockaddr_storage saddr;
>> -socklen_t len;
>> -int fd;
>> -
>> -for (;;) {
>> -len = sizeof(saddr);
>> -fd = qemu_accept(s->listen_fd, (struct sockaddr *), );
>> -if (fd < 0 && errno != EINTR) {
>> -return;
>> -} else if (fd >= 0) {
>> -qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
>> -break;
>> -}
>> -}
>> +QIOChannelSocket *listen_sioc = QIO_CHANNEL_SOCKET(s->listen_ioc);
>> +SocketAddress *addr;
>> +int ret;
>>   -s->fd = fd;
>> -s->nc.link_down = false;
>> -net_stream_connect(s);
>> -switch (saddr.ss_family) {
>> -case AF_INET: {
>> -struct sockaddr_in *saddr_in = (struct sockaddr_in *)
>> -
>> -qemu_set_info_str(>nc, "connection from %s:%d",
>> -  inet_ntoa(saddr_in->sin_addr),
>> -  ntohs(saddr_in->sin_port));
>> -break;
>> +if (listen_sioc->fd < 0) {
>> +qemu_set_info_str(>nc, "connection error");
>> +return;
>>   }
>> -case AF_UNIX: {
>> -struct sockaddr_un saddr_un;
>>   -len = sizeof(saddr_un);
>> -getsockname(s->listen_fd, (struct sockaddr *)_un, );
>> -qemu_set_info_str(>nc, "connect from %s", saddr_un.sun_path);
>> -break;
>> -}
>> -default:
>> -g_assert_not_reached();
>> +addr = qio_channel_socket_get_local_address(listen_sioc, NULL);
>> +g_assert(addr != NULL);
>
> Missing propagating Error* (observed in v12).

*If* this is really a programming error: what about _abort?

[...]

Re: [PATCH v14 15/17] net: stream: move to QIO to enable additional parameters

2022-10-21 Thread Philippe Mathieu-Daudé 

On 21/10/22 11:09, Laurent Vivier wrote:

Use QIOChannel, QIOChannelSocket and QIONetListener.
This allows net/stream to use all the available parameters provided by
SocketAddress.

Signed-off-by: Laurent Vivier 
Acked-by: Michael S. Tsirkin 
---
  net/stream.c| 492 +---
  qemu-options.hx |   4 +-
  2 files changed, 178 insertions(+), 318 deletions(-)



-static void net_stream_accept(void *opaque)
+static void net_stream_server_listening(QIOTask *task, gpointer opaque)
  {
  NetStreamState *s = opaque;
-struct sockaddr_storage saddr;
-socklen_t len;
-int fd;
-
-for (;;) {
-len = sizeof(saddr);
-fd = qemu_accept(s->listen_fd, (struct sockaddr *), );
-if (fd < 0 && errno != EINTR) {
-return;
-} else if (fd >= 0) {
-qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
-break;
-}
-}
+QIOChannelSocket *listen_sioc = QIO_CHANNEL_SOCKET(s->listen_ioc);
+SocketAddress *addr;
+int ret;
  
-s->fd = fd;

-s->nc.link_down = false;
-net_stream_connect(s);
-switch (saddr.ss_family) {
-case AF_INET: {
-struct sockaddr_in *saddr_in = (struct sockaddr_in *)
-
-qemu_set_info_str(>nc, "connection from %s:%d",
-  inet_ntoa(saddr_in->sin_addr),
-  ntohs(saddr_in->sin_port));
-break;
+if (listen_sioc->fd < 0) {
+qemu_set_info_str(>nc, "connection error");
+return;
  }
-case AF_UNIX: {
-struct sockaddr_un saddr_un;
  
-len = sizeof(saddr_un);

-getsockname(s->listen_fd, (struct sockaddr *)_un, );
-qemu_set_info_str(>nc, "connect from %s", saddr_un.sun_path);
-break;
-}
-default:
-g_assert_not_reached();
+addr = qio_channel_socket_get_local_address(listen_sioc, NULL);
+g_assert(addr != NULL);


Missing propagating Error* (observed in v12).


+ret = qemu_socket_try_set_nonblock(listen_sioc->fd);
+if (addr->type == SOCKET_ADDRESS_TYPE_FD && ret < 0) {
+qemu_set_info_str(>nc, "can't use file descriptor %s (errno %d)",
+  addr->u.fd.str, -ret);
+return;
  }
+g_assert(ret == 0);
+qapi_free_SocketAddress(addr);
+
+s->nc.link_down = true;
+s->listener = qio_net_listener_new();
+
+net_socket_rs_init(>rs, net_stream_rs_finalize, false);
+qio_net_listener_set_client_func(s->listener, net_stream_listen, s, NULL);
+qio_net_listener_add(s->listener, listen_sioc);
  }
  
  static int net_stream_server_init(NetClientState *peer,

@@ -283,105 +286,61 @@ static int net_stream_server_init(NetClientState *peer,
  {
  NetClientState *nc;
  NetStreamState *s;
-int fd, ret;
+QIOChannelSocket *listen_sioc = qio_channel_socket_new();
  
-switch (addr->type) {

-case SOCKET_ADDRESS_TYPE_INET: {
-struct sockaddr_in saddr_in;
-
-if (convert_host_port(_in, addr->u.inet.host, addr->u.inet.port,
-  errp) < 0) {
-return -1;
-}
-
-fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
-if (fd < 0) {
-error_setg_errno(errp, errno, "can't create stream socket");
-return -1;
-}
-qemu_socket_set_nonblock(fd);
+nc = qemu_new_net_client(_stream_info, peer, model, name);
+s = DO_UPCAST(NetStreamState, nc, nc);
  
-socket_set_fast_reuse(fd);

+s->listen_ioc = QIO_CHANNEL(listen_sioc);
+qio_channel_socket_listen_async(listen_sioc, addr, 0,
+net_stream_server_listening, s,
+NULL, NULL);
  
-ret = bind(fd, (struct sockaddr *)_in, sizeof(saddr_in));

-if (ret < 0) {
-error_setg_errno(errp, errno, "can't bind ip=%s to socket",
- inet_ntoa(saddr_in.sin_addr));
-closesocket(fd);
-return -1;
-}
-break;
-}
-case SOCKET_ADDRESS_TYPE_UNIX: {
-struct sockaddr_un saddr_un;
-
-ret = unlink(addr->u.q_unix.path);
-if (ret < 0 && errno != ENOENT) {
-error_setg_errno(errp, errno, "failed to unlink socket %s",
- addr->u.q_unix.path);
-return -1;
-}
+return 0;
+}
  
-saddr_un.sun_family = PF_UNIX;

-ret = snprintf(saddr_un.sun_path, sizeof(saddr_un.sun_path), "%s",
-   addr->u.q_unix.path);
-if (ret < 0 || ret >= sizeof(saddr_un.sun_path)) {
-error_setg(errp, "UNIX socket path '%s' is too long",
-   addr->u.q_unix.path);
-error_append_hint(errp, "Path must be less than %zu bytes\n",
-  sizeof(saddr_un.sun_path));
-return -1;
-}
+static void net_stream_client_connected(QIOTask *task, gpointer opaque)
+{
+NetStreamState *s