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

[Markus Armbruster <armbru@xxxxxxxxxx>]

Laurent Vivier <lvivier@xxxxxxxxxx> writes:

> On 10/21/22 12:35, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> 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 <lvivier@xxxxxxxxxx>
>>>> Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
>>>> ---
>>>>    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 *)&saddr, &len);
>>>> -        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 *)&saddr;
>>>> -
>>>> -        qemu_set_info_str(&s->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(&s->nc, "connection error");
>>>> +        return;
>>>>        }
>>>> -    case AF_UNIX: {
>>>> -        struct sockaddr_un saddr_un;
>>>> -        len = sizeof(saddr_un);
>>>> -        getsockname(s->listen_fd, (struct sockaddr *)&saddr_un, &len);
>>>> -        qemu_set_info_str(&s->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 &error_abort?
>
> assert() informs the compiler that following code will not use addr with a 
> NULL value, I 
> don't think &error_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 &error_abort
outside tests/.  I'm not aware of them confusing static analyzers we
care about.

I like &error_abort, because it makes the program crash when we try to
put the error into &error_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 &error_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.