Re: [PATCH] chardev: report the handshake error

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, May 10, 2023 at 12:34:59PM +0200, Markus Armbruster wrote:
> 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?

Yes, from chardev-add.

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

It isn't, as chardev-add has already completed and returned
"success" to the client at this point IIRC.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|