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

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



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.




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.