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

On Mon, Sep 21, 2015 at 17:32:41 -0400, John Ferlan wrote: > > > 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. I think /* coverity[dead_error_begin] */ is the right solution in this case.