Re: [libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function

[John Ferlan]

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> There's no reason why the virNetServerClientDispatchRead method needs to
> acquire an extra reference on the "client" object. An extra reference is
> only needed if the registered dispatch callback is going to keep hold of
> the "client" for work in the background. Thus we can push reference
> acquisition into virNetServerDispatchNewMessage.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
>  src/rpc/virnetserver.c   | 2 ++
>  src/rpc/virnetserverclient.c | 2 --
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Makes sense... Chasing for the Unref shows virNetServerHandleJob which
of course had me wondering for the non dispatch path, when was that
Unref done in virNetServerClientDispatchRead...


Reviewed-by: John Ferlan 

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function

[Jim Fehlig]

On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:

There's no reason why the virNetServerClientDispatchRead method needs to
acquire an extra reference on the "client" object. An extra reference is
only needed if the registered dispatch callback is going to keep hold of
the "client" for work in the background. Thus we can push reference
acquisition into virNetServerDispatchNewMessage.

Signed-off-by: Daniel P. Berrangé 
---
  src/rpc/virnetserver.c   | 2 ++
  src/rpc/virnetserverclient.c | 2 --
  2 files changed, 2 insertions(+), 2 deletions(-)


I haven't worked on this code much, but this change seems sane to me regardless 
of the lost locks issue.


Reviewed-by: Jim Fehlig 

Regards,
Jim



diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 204f425264..28afe54e49 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -217,9 +217,11 @@ static int 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
  priority = virNetServerProgramGetPriority(prog, msg->header.proc);
  }
  
+virObjectRef(client);

  ret = virThreadPoolSendJob(srv->workers, priority, job);
  
  if (ret < 0) {

+virObjectUnref(client);
  VIR_FREE(job);
  virObjectUnref(prog);
  }
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 388514946b..00459d17ba 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1315,12 +1315,10 @@ static void 
virNetServerClientDispatchRead(virNetServerClientPtr client)
  
  /* Send off to for normal dispatch to workers */

  if (msg) {
-virObjectRef(client);
  if (!client->dispatchFunc ||
  client->dispatchFunc(client, msg, client->dispatchOpaque) < 
0) {
  virNetMessageFree(msg);
  client->wantClose = true;
-virObjectUnref(client);
  return;
  }
  }



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/5] rpc: push ref acquisition into RPC dispatch function

[Daniel P . Berrangé]

There's no reason why the virNetServerClientDispatchRead method needs to
acquire an extra reference on the "client" object. An extra reference is
only needed if the registered dispatch callback is going to keep hold of
the "client" for work in the background. Thus we can push reference
acquisition into virNetServerDispatchNewMessage.

Signed-off-by: Daniel P. Berrangé 
---
 src/rpc/virnetserver.c   | 2 ++
 src/rpc/virnetserverclient.c | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 204f425264..28afe54e49 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -217,9 +217,11 @@ static int 
virNetServerDispatchNewMessage(virNetServerClientPtr client,
 priority = virNetServerProgramGetPriority(prog, msg->header.proc);
 }
 
+virObjectRef(client);
 ret = virThreadPoolSendJob(srv->workers, priority, job);
 
 if (ret < 0) {
+virObjectUnref(client);
 VIR_FREE(job);
 virObjectUnref(prog);
 }
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 388514946b..00459d17ba 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1315,12 +1315,10 @@ static void 
virNetServerClientDispatchRead(virNetServerClientPtr client)
 
 /* Send off to for normal dispatch to workers */
 if (msg) {
-virObjectRef(client);
 if (!client->dispatchFunc ||
 client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) 
{
 virNetMessageFree(msg);
 client->wantClose = true;
-virObjectUnref(client);
 return;
 }
 }
-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list