Currently virNetServerClientDispatchFunc implementations are only
responsible for free'ing the "msg" parameter upon success. Simplify the
calling convention by making it their unconditional responsibility to
free the "msg", and close the client if desired.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetserver.c       | 24 +++++++++++++-----------
 src/rpc/virnetserverclient.c |  6 +++---
 src/rpc/virnetserverclient.h |  9 ++++++---
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 28afe54e49..7a1376bf49 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -182,15 +182,14 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
     VIR_FREE(job);
 }
 
-static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
-                                          virNetMessagePtr msg,
-                                          void *opaque)
+static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
+                                           virNetMessagePtr msg,
+                                           void *opaque)
 {
     virNetServerPtr srv = opaque;
     virNetServerProgramPtr prog = NULL;
     unsigned int priority = 0;
     size_t i;
-    int ret = -1;
 
     VIR_DEBUG("server=%p client=%p message=%p",
               srv, client, msg);
@@ -207,7 +206,7 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
         virNetServerJobPtr job;
 
         if (VIR_ALLOC(job) < 0)
-            goto cleanup;
+            goto error;
 
         job->client = client;
         job->msg = msg;
@@ -218,21 +217,24 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
         }
 
         virObjectRef(client);
-        ret = virThreadPoolSendJob(srv->workers, priority, job);
-
-        if (ret < 0) {
+        if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
             virObjectUnref(client);
             VIR_FREE(job);
             virObjectUnref(prog);
+            goto error;
         }
     } else {
-        ret = virNetServerProcessMsg(srv, client, prog, msg);
+        if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
+            goto error;
     }
 
- cleanup:
     virObjectUnlock(srv);
+    return;
 
-    return ret;
+ error:
+    virNetMessageFree(msg);
+    virNetServerClientClose(client);
+    virObjectUnlock(srv);
 }
 
 /**
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index 00459d17ba..ea0d5abdee 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -1315,11 +1315,11 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
 
         /* Send off to for normal dispatch to workers */
         if (msg) {
-            if (!client->dispatchFunc ||
-                client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
+            if (!client->dispatchFunc) {
                 virNetMessageFree(msg);
                 client->wantClose = true;
-                return;
+            } else {
+                client->dispatchFunc(client, msg, client->dispatchOpaque);
             }
         }
 
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 1a939ad4e1..b21446eeb7 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -36,9 +36,12 @@ typedef virNetServer *virNetServerPtr;
 typedef struct _virNetServerClient virNetServerClient;
 typedef virNetServerClient *virNetServerClientPtr;
 
-typedef int (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
-                                              virNetMessagePtr msg,
-                                              void *opaque);
+/* This function owns the "msg" pointer it is passed and
+ * must arrange for virNetMessageFree to be called on it
+ */
+typedef void (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
+                                               virNetMessagePtr msg,
+                                               void *opaque);
 
 typedef int (*virNetServerClientFilterFunc)(virNetServerClientPtr client,
                                             virNetMessagePtr msg,
-- 
2.14.3

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

On 03/06/2018 12:58 PM, Daniel P. Berrangé wrote:
> Currently virNetServerClientDispatchFunc implementations are only
> responsible for free'ing the "msg" parameter upon success. Simplify the
> calling convention by making it their unconditional responsibility to
> free the "msg", and close the client if desired.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/rpc/virnetserver.c       | 24 +++++++++++++-----------
>  src/rpc/virnetserverclient.c |  6 +++---
>  src/rpc/virnetserverclient.h |  9 ++++++---
>  3 files changed, 22 insertions(+), 17 deletions(-)
> 

Chasing all the error paths for @msg to be freed is mind boggling ;-)

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

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

On 03/06/2018 10:58 AM, Daniel P. Berrangé wrote:
> Currently virNetServerClientDispatchFunc implementations are only
> responsible for free'ing the "msg" parameter upon success. Simplify the
> calling convention by making it their unconditional responsibility to
> free the "msg", and close the client if desired.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetserver.c       | 24 +++++++++++++-----------
>   src/rpc/virnetserverclient.c |  6 +++---
>   src/rpc/virnetserverclient.h |  9 ++++++---
>   3 files changed, 22 insertions(+), 17 deletions(-)

Same as 1/1. Looks like a nice improvement regardless of lost locks.

Reviewed-by: Jim Fehlig <jfehlig@suse.com>

Regards,
Jim

> 
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 28afe54e49..7a1376bf49 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -182,15 +182,14 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque)
>       VIR_FREE(job);
>   }
>   
> -static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
> -                                          virNetMessagePtr msg,
> -                                          void *opaque)
> +static void virNetServerDispatchNewMessage(virNetServerClientPtr client,
> +                                           virNetMessagePtr msg,
> +                                           void *opaque)
>   {
>       virNetServerPtr srv = opaque;
>       virNetServerProgramPtr prog = NULL;
>       unsigned int priority = 0;
>       size_t i;
> -    int ret = -1;
>   
>       VIR_DEBUG("server=%p client=%p message=%p",
>                 srv, client, msg);
> @@ -207,7 +206,7 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
>           virNetServerJobPtr job;
>   
>           if (VIR_ALLOC(job) < 0)
> -            goto cleanup;
> +            goto error;
>   
>           job->client = client;
>           job->msg = msg;
> @@ -218,21 +217,24 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr client,
>           }
>   
>           virObjectRef(client);
> -        ret = virThreadPoolSendJob(srv->workers, priority, job);
> -
> -        if (ret < 0) {
> +        if (virThreadPoolSendJob(srv->workers, priority, job) < 0) {
>               virObjectUnref(client);
>               VIR_FREE(job);
>               virObjectUnref(prog);
> +            goto error;
>           }
>       } else {
> -        ret = virNetServerProcessMsg(srv, client, prog, msg);
> +        if (virNetServerProcessMsg(srv, client, prog, msg) < 0)
> +            goto error;
>       }
>   
> - cleanup:
>       virObjectUnlock(srv);
> +    return;
>   
> -    return ret;
> + error:
> +    virNetMessageFree(msg);
> +    virNetServerClientClose(client);
> +    virObjectUnlock(srv);
>   }
>   
>   /**
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 00459d17ba..ea0d5abdee 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -1315,11 +1315,11 @@ static void virNetServerClientDispatchRead(virNetServerClientPtr client)
>   
>           /* Send off to for normal dispatch to workers */
>           if (msg) {
> -            if (!client->dispatchFunc ||
> -                client->dispatchFunc(client, msg, client->dispatchOpaque) < 0) {
> +            if (!client->dispatchFunc) {
>                   virNetMessageFree(msg);
>                   client->wantClose = true;
> -                return;
> +            } else {
> +                client->dispatchFunc(client, msg, client->dispatchOpaque);
>               }
>           }
>   
> diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
> index 1a939ad4e1..b21446eeb7 100644
> --- a/src/rpc/virnetserverclient.h
> +++ b/src/rpc/virnetserverclient.h
> @@ -36,9 +36,12 @@ typedef virNetServer *virNetServerPtr;
>   typedef struct _virNetServerClient virNetServerClient;
>   typedef virNetServerClient *virNetServerClientPtr;
>   
> -typedef int (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
> -                                              virNetMessagePtr msg,
> -                                              void *opaque);
> +/* This function owns the "msg" pointer it is passed and
> + * must arrange for virNetMessageFree to be called on it
> + */
> +typedef void (*virNetServerClientDispatchFunc)(virNetServerClientPtr client,
> +                                               virNetMessagePtr msg,
> +                                               void *opaque);
>   
>   typedef int (*virNetServerClientFilterFunc)(virNetServerClientPtr client,
>                                               virNetMessagePtr msg,
> 

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