Laurent Vivier <lviv...@redhat.com> writes:
> On 10/21/22 12:35, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <phi...@linaro.org> 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 <lviv...@redhat.com>
>>>> Acked-by: Michael S. Tsirkin <m...@redhat.com>
>>>> ---
>>>> 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.
