Hi On Wed, May 10, 2023 at 1:39 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
> On Wed, May 10, 2023 at 11:31:40AM +0200, Markus Armbruster wrote: > > marcandre.lur...@redhat.com writes: > > > > > From: Marc-André Lureau <marcandre.lur...@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.lur...@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: > I guess it depends on usage, if you have a reconnect= option, then it can be considered non-fatal and a warning is fine. Should we check if there is a reconnect to decide whether to print an error or a warning? no strong opinion.. > > > 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 :| > > > -- Marc-André Lureau
