Re: [PATCH 2/5] rpc: simplify calling convention of virNetServerClientDispatchFunc

[Jim Fehlig <jfehlig@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
>   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@xxxxxxxx>

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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list