Re: [PATCH] chardev: report the handshake error

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, May 10, 2023 at 11:31:40AM +0200, Markus Armbruster wrote:
>> marcandre.lureau@redhat.com writes:
>> 
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > This can help to debug connection issues.
>> >
>> > Related to:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=2196182
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  chardev/char-socket.c | 12 ++++++++++--
>> >  1 file changed, 10 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> > index 8c58532171..e8e3a743d5 100644
>> > --- a/chardev/char-socket.c
>> > +++ b/chardev/char-socket.c
>> > @@ -742,8 +742,12 @@ static void tcp_chr_websock_handshake(QIOTask *task, 
>> > gpointer user_data)
>> >  {
>> >      Chardev *chr = user_data;
>> >      SocketChardev *s = user_data;
>> > +    Error *err = NULL;
>> >  
>> > -    if (qio_task_propagate_error(task, NULL)) {
>> > +    if (qio_task_propagate_error(task, &err)) {
>> > +        error_reportf_err(err,
>> > +                          "websock handshake of character device %s 
>> > failed: ",
>> > +                          chr->label);
>> 
>> Code smell: reports an error without failing the function.
>> 
>> Should it be a warning instead?
>
> Well it isn't a warning, this is a fatal error wrt continued use
> of the chardev
>
> Not failing the function is expected in this particular code
> pattern. These tcp_chr_(tls,websock)_handshake functions are
> callbacks that are used to handle an async operations progress.
> From the caller's POV, it doesn't matter whether there is an
> error or success. It is upto this function to do whatever is
> required based on the status, hence the call to disconnect
> the chardev on error:
>
>> >          tcp_chr_disconnect(chr);

Can this asynchronous task be started from QMP?

If yes, how is this error reported back to the QMP client?

[...]