Re: [libvirt] [PATCH 3/3] virsh: Notify users about disconnects

On 09/17/2015 08:23 AM, Jiri Denemark wrote: > After my "client rpc: Report proper error for keepalive disconnections" > patch, virsh would no long print a warning when it closes a connection > to a daemon after a keepalive timeout. Although the warning > > virsh # 2015-09-15 10:59:26.729+0000: 642080: info : > libvirt version: 1.2.19 > 2015-09-15 10:59:26.729+0000: 642080: warning : > virKeepAliveTimerInternal:143 : No response from client > 0x7efdc0a46730 after 1 keepalive messages in 2 seconds > > was pretty ugly, it was still useful. This patch brings the useful part > back while making it much nicer: > > virsh # error: Disconnected from qemu:///system due to keepalive timeout > > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; > --- > tools/virsh.c | 35 ++++++++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > Patch series seems reasonable to me, except for one minor thing.... Found by my coverity checker... Naturally > diff --git a/tools/virsh.c b/tools/virsh.c > index bb12dec..23436ea 100644 > --- a/tools/virsh.c > +++ b/tools/virsh.c > @@ -95,12 +95,41 @@ static int disconnected; /* we may have been disconnected */ > * handler, just save the fact it was raised. > */ > static void > -virshCatchDisconnect(virConnectPtr conn ATTRIBUTE_UNUSED, > +virshCatchDisconnect(virConnectPtr conn, > int reason, > - void *opaque ATTRIBUTE_UNUSED) > + void *opaque) > { > - if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT) > + if (reason != VIR_CONNECT_CLOSE_REASON_CLIENT) { [1] > + vshControl *ctl = opaque; > + const char *str = "unknown reason"; > + virErrorPtr error; > + char *uri; > + > + error = virSaveLastError(); > + uri = virConnectGetURI(conn); > + > + switch ((virConnectCloseReason) reason) { > + case VIR_CONNECT_CLOSE_REASON_ERROR: > + str = N_("Disconnected from %s due to I/O error"); > + break; > + case VIR_CONNECT_CLOSE_REASON_EOF: > + str = N_("Disconnected from %s due to end of file"); > + break; > + case VIR_CONNECT_CLOSE_REASON_KEEPALIVE: > + str = N_("Disconnected from %s due to keepalive timeout"); > + break; > + case VIR_CONNECT_CLOSE_REASON_CLIENT: [1] ^^^ Coverity complains about DEADCODE Other than setting str = NULL and testing for it later, I'm not exactly sure of the 'best' course of action. e.g., if (str) { vshError(); disconnected++; } I think if this is handled, then the series is ACK-able.