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

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 21/10/22 13:31, Markus Armbruster wrote:
> 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(-)
> > > > > +    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.
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-0c0d8fa3494f@xxxxxxxxxx/

> 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.