Re: [PATCH] chardev: report the handshake error

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

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);
> >      } else {
> >          if (s->do_telnetopt) {
> > @@ -778,8 +782,12 @@ static void tcp_chr_tls_handshake(QIOTask *task,
> >  {
> >      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,
> > +                          "TLS handshake of character device %s failed: ",
> > +                          chr->label);
> >          tcp_chr_disconnect(chr);
> >      } else {
> >          if (s->is_websock) {
> 
> Likewise.
> 

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