Message ID | 1412618363-14968-2-git-send-email-minyard@acm.org |
---|---|
State | New |
Headers | show |
On 10/06/2014 11:59 AM, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > If reconnect was set, errors wouldn't always be reported. > Fix that and also only report a connect error once until a > connection has been made. > > The primary purpose of this is to tell the user that a > connection failed so they can know they need to figure out > what went wrong. So we don't want to spew too much > out here, just enough so they know. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > qemu-char.c | 47 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 32 insertions(+), 15 deletions(-) > + bool connect_err_reported; > } TCPCharDriver; > > static gboolean socket_reconnect_timeout(gpointer opaque); > @@ -2521,6 +2522,17 @@ static void qemu_chr_socket_restart_timer(CharDriverState *chr) > socket_reconnect_timeout, chr); > } > > +static void check_report_connect_error(CharDriverState *chr, const char *str) > +{ > + TCPCharDriver *s = chr->opaque; > + > + if (!s->connect_err_reported) { > + error_report("%s char device %s\n", str, chr->label); No \n at the end of an error_report() message. > + s->connect_err_reported = 1; s/1/true/ since you typed it as bool. > + } > + qemu_chr_socket_restart_timer(chr); > +} > + > static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque); > > #ifndef _WIN32 > @@ -3045,12 +3057,14 @@ static void qemu_chr_finish_socket_connection(CharDriverState *chr, int fd) > static void qemu_chr_socket_connected(int fd, void *opaque) > { > CharDriverState *chr = opaque; > + TCPCharDriver *s = chr->opaque; > > if (fd < 0) { > - qemu_chr_socket_restart_timer(chr); > + check_report_connect_error(chr, "Unable to connect to"); Works in English, but if there is ever translation of the message printed in check_report_connect_error, you are probably doing translators a disservice by splitting a sentence into two parts separated by a number of lines of code. (Spoken by one who has never contributed a translation, so take with a grain of salt) > return; > } > > + s->connect_err_reported = 0; s/0/false/
On 10/06/2014 01:36 PM, Eric Blake wrote: > On 10/06/2014 11:59 AM, minyard@acm.org wrote: >> From: Corey Minyard <cminyard@mvista.com> >> >> If reconnect was set, errors wouldn't always be reported. >> Fix that and also only report a connect error once until a >> connection has been made. >> >> The primary purpose of this is to tell the user that a >> connection failed so they can know they need to figure out >> what went wrong. So we don't want to spew too much >> out here, just enough so they know. >> >> Signed-off-by: Corey Minyard <cminyard@mvista.com> >> --- >> qemu-char.c | 47 ++++++++++++++++++++++++++++++++--------------- >> 1 file changed, 32 insertions(+), 15 deletions(-) >> + bool connect_err_reported; >> } TCPCharDriver; >> >> static gboolean socket_reconnect_timeout(gpointer opaque); >> @@ -2521,6 +2522,17 @@ static void qemu_chr_socket_restart_timer(CharDriverState *chr) >> socket_reconnect_timeout, chr); >> } >> >> +static void check_report_connect_error(CharDriverState *chr, const char *str) >> +{ >> + TCPCharDriver *s = chr->opaque; >> + >> + if (!s->connect_err_reported) { >> + error_report("%s char device %s\n", str, chr->label); > No \n at the end of an error_report() message. Oops, I read that and forgot to change it. > >> + s->connect_err_reported = 1; > s/1/true/ since you typed it as bool. Sigh. Old habits die hard. > >> + } >> + qemu_chr_socket_restart_timer(chr); >> +} >> + >> static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque); >> >> #ifndef _WIN32 >> @@ -3045,12 +3057,14 @@ static void qemu_chr_finish_socket_connection(CharDriverState *chr, int fd) >> static void qemu_chr_socket_connected(int fd, void *opaque) >> { >> CharDriverState *chr = opaque; >> + TCPCharDriver *s = chr->opaque; >> >> if (fd < 0) { >> - qemu_chr_socket_restart_timer(chr); >> + check_report_connect_error(chr, "Unable to connect to"); > Works in English, but if there is ever translation of the message > printed in check_report_connect_error, you are probably doing > translators a disservice by splitting a sentence into two parts > separated by a number of lines of code. (Spoken by one who has never > contributed a translation, so take with a grain of salt) Yeah, I was avoiding having to add an error_vreport(). I should just add that. All other comments fixed. Thanks. -corey >> return; >> } >> >> + s->connect_err_reported = 0; > s/0/false/ > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Il 06/10/2014 20:36, Eric Blake ha scritto: >>> - qemu_chr_socket_restart_timer(chr); + >>> check_report_connect_error(chr, "Unable to connect to"); > Works in English, but if there is ever translation of the message > printed in check_report_connect_error, you are probably doing > translators a disservice by splitting a sentence into two parts > separated by a number of lines of code. (Spoken by one who has > never contributed a translation, so take with a grain of salt) > I agree, I think it's simpler to unify the two messages. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBAgAGBQJUMv1IAAoJEBRUblpOawnXtFoH/1QIYr/yZXZQhw7zqDxiyhgH Gto3UiDjd/eLNC/iFwdY3J2pnuHIu8yVmBAO68cALdK6KotZ6X/TGVpsGUmPeoPk rPtkbxAvBvxdPImCUi798YSXbkOsVA/5+pMOOxNLeMwzA2ClFDFBqzgqgBbL/PwU O25XgCNucwFpk6An5yTOblHbS5Ji6oSnfJfHkXwuZlzjE7ql2nbr0Gr9lEulgHk3 3+hDoyesCD55V0r1qO/CwGJsuyJ1PXJGxV65ML1ja6F+yiMjlCgaTmzLAtcdWu4A oD6wR5auS9ZTWogS/JBxCx9oFnZVs19LgnSPSvQVHb3TaQJcVSTZuQjkb3/m8bU= =ctwR -----END PGP SIGNATURE-----
minyard@acm.org writes: > From: Corey Minyard <cminyard@mvista.com> > > If reconnect was set, errors wouldn't always be reported. > Fix that and also only report a connect error once until a > connection has been made. > > The primary purpose of this is to tell the user that a > connection failed so they can know they need to figure out > what went wrong. So we don't want to spew too much > out here, just enough so they know. > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > --- > qemu-char.c | 47 ++++++++++++++++++++++++++++++++--------------- > 1 file changed, 32 insertions(+), 15 deletions(-) > > diff --git a/qemu-char.c b/qemu-char.c > index 62af0ef..fb895c7 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -2509,6 +2509,7 @@ typedef struct { > > guint reconnect_timer; > int64_t reconnect_time; > + bool connect_err_reported; > } TCPCharDriver; > > static gboolean socket_reconnect_timeout(gpointer opaque); Doesn't apply, obviously depends on some other patch. Always state your dependencies explicitly in the cover letter! [...]
diff --git a/qemu-char.c b/qemu-char.c index 62af0ef..fb895c7 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2509,6 +2509,7 @@ typedef struct { guint reconnect_timer; int64_t reconnect_time; + bool connect_err_reported; } TCPCharDriver; static gboolean socket_reconnect_timeout(gpointer opaque); @@ -2521,6 +2522,17 @@ static void qemu_chr_socket_restart_timer(CharDriverState *chr) socket_reconnect_timeout, chr); } +static void check_report_connect_error(CharDriverState *chr, const char *str) +{ + TCPCharDriver *s = chr->opaque; + + if (!s->connect_err_reported) { + error_report("%s char device %s\n", str, chr->label); + s->connect_err_reported = 1; + } + qemu_chr_socket_restart_timer(chr); +} + static gboolean tcp_chr_accept(GIOChannel *chan, GIOCondition cond, void *opaque); #ifndef _WIN32 @@ -3045,12 +3057,14 @@ static void qemu_chr_finish_socket_connection(CharDriverState *chr, int fd) static void qemu_chr_socket_connected(int fd, void *opaque) { CharDriverState *chr = opaque; + TCPCharDriver *s = chr->opaque; if (fd < 0) { - qemu_chr_socket_restart_timer(chr); + check_report_connect_error(chr, "Unable to connect to"); return; } + s->connect_err_reported = 0; qemu_chr_finish_socket_connection(chr, fd); } @@ -4066,11 +4080,19 @@ static CharDriverState *qmp_chardev_open_parallel(ChardevHostdev *parallel, #endif /* WIN32 */ +static void socket_try_connect(CharDriverState *chr) +{ + Error *err = NULL; + + if (!qemu_chr_open_socket_fd(chr, &err)) { + check_report_connect_error(chr, "Unable to start connection to"); + } +} + static gboolean socket_reconnect_timeout(gpointer opaque) { CharDriverState *chr = opaque; TCPCharDriver *s = chr->opaque; - Error *err; s->reconnect_timer = 0; @@ -4078,10 +4100,7 @@ static gboolean socket_reconnect_timeout(gpointer opaque) return false; } - if (!qemu_chr_open_socket_fd(chr, &err)) { - error_report("Unable to connect to char device %s\n", chr->label); - qemu_chr_socket_restart_timer(chr); - } + socket_try_connect(chr); return false; } @@ -4133,15 +4152,13 @@ static CharDriverState *qmp_chardev_open_socket(ChardevSocket *sock, s->reconnect_time = reconnect; } - if (!qemu_chr_open_socket_fd(chr, errp)) { - if (s->reconnect_time) { - qemu_chr_socket_restart_timer(chr); - } else { - g_free(s); - g_free(chr->filename); - g_free(chr); - return NULL; - } + if (s->reconnect_time) { + socket_try_connect(chr); + } else if (!qemu_chr_open_socket_fd(chr, errp)) { + g_free(s); + g_free(chr->filename); + g_free(chr); + return NULL; } if (is_listen && is_waitconnect) {