<div dir="ltr"><div><br></div>I looked through what you were suggesting.<div>I was assuming <span style="font-family:menlo;font-size:14px;font-variant-ligatures:no-common-ligatures">virNetSocketGetFD()</span>would do a NULL check for the sock arg, and would return immediately if a different client executed a <span style="font-family:menlo;font-size:14px;font-variant-ligatures:no-common-ligatures">virNetServerClientClose()</span> setting client->sock to null<font face="menlo"><span style="font-size:14px;font-variant-ligatures:no-common-ligatures">.<br><br></span></font></div>







<div>Since this check is missing, I understand the implicit assumption is to always have virNetServerClientGetFD() lock the client, and consequently, I will rearrange debug messages around to prevent lock contention.<br></div><div><br></div><div>Sending out a V2.</div>







</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 22, 2017 at 1:41 PM, Peter Krempa <span dir="ltr"><<a href="mailto:pkrempa@redhat.com" target="_blank">pkrempa@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Mar 22, 2017 at 01:02:17 -0700, Prerna Saxena wrote:<br>
> While tracing connections from a remote client, it helps to keep track<br>
> of the connection lifecycle. Messages such as the following :<br>
><br>
> error : virNetSocketReadWire:1574 : End of file while reading data: Input/output error<br>
><br>
> are rather unhelpful. They do not indicate if the client had earlier asked for<br>
> connection closure via libvirt API.<br>
> This patch introduces messages to annotate when a client connected/disconnected.<br>
><br>
> Signed-off-by: Prerna Saxena <<a href="mailto:saxenap.ltc@gmail.com">saxenap.ltc@gmail.com</a>><br>
> ---<br>
>  src/rpc/virnetserverclient.c | 18 ++++++++++++++----<br>
>  1 file changed, 14 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c<br>
> index 85857bc..a77feaa 100644<br>
> --- a/src/rpc/virnetserverclient.c<br>
> +++ b/src/rpc/virnetserverclient.c<br>
> @@ -678,14 +678,19 @@ int virNetServerClientGetTLSKeySiz<wbr>e(virNetServerClientPtr client)<br>
>      return size;<br>
>  }<br>
>  #endif<br>
> -<br>
> +/*<br>
> + * This mostly just needs to publish the client socket FD to logs.<br>
> + * It does not necessarily need a lock, or will add lock contention problems.<br>
> + * Replace the lock with a reference counting mechanism, to prevent the client<br>
> + * object from being deallocated while this is being run<br>
> + */<br>
>  int virNetServerClientGetFD(<wbr>virNetServerClientPtr client)<br>
>  {<br>
>      int fd = -1;<br>
> -    virObjectLock(client);<br>
> +    virObjectRef(client);<br>
>      if (client->sock)<br>
>          fd = virNetSocketGetFD(client-><wbr>sock);<br>
> -    virObjectUnlock(client);<br>
> +    virObjectUnref(client);<br>
<br>
</div></div>This change is not justified in any way. Also looks wrong. You can't<br>
access an unlocked object.<br>
<span class=""><br>
>      return fd;<br>
>  }<br>
><br>
> @@ -965,7 +970,9 @@ void virNetServerClientClose(<wbr>virNetServerClientPtr client)<br>
>      virKeepAlivePtr ka;<br>
><br>
>      virObjectLock(client);<br>
> -    VIR_DEBUG("client=%p", client);<br>
> +    VIR_WARN("Free'ing up resources for client=%p sock=%d", client,<br>
> +               virNetServerClientGetFD(<wbr>client));<br>
<br>
</span>NACK using warnings instead of debug messages is not desired. For debug<br>
purposes you should use debug logs.<br>
</blockquote></div><br></div>