[libvirt] [PATCH v2 08/10] admin: Introduce virAdmServerGethreadPoolParameters

[Michal Privoznik]

On 29.03.2016 09:51, Erik Skultety wrote:
> New API to retrieve current server workerpool specs. Since it uses typed
> parameters, more specs to retrieve can be further included in the pool of
> supported ones.
> ---
>  daemon/admin.c                  | 59 +++++++++++++++++++++++++++++++++++
>  daemon/admin_server.c           | 69 +++++++++++++++++++++++++++++++++++++++++
>  daemon/admin_server.h           |  6 ++++
>  include/libvirt/libvirt-admin.h |  6 ++++
>  po/POTFILES.in                  |  1 +
>  src/admin/admin_protocol.x      | 20 ++++++++++--
>  src/admin/admin_remote.c        | 43 +++++++++++++++++++++++++
>  src/admin_protocol-structs      | 11 +++++++
>  src/libvirt-admin.c             | 46 +++++++++++++++++++++++++++
>  src/libvirt_admin_private.syms  |  2 ++
>  src/libvirt_admin_public.syms   |  1 +
>  src/rpc/virnetserver.c          | 22 +++++++++++++
>  src/rpc/virnetserver.h          | 13 ++++++++
>  13 files changed, 296 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/admin.c b/daemon/admin.c
> index 3169cdd..5335cce 100644
> --- a/daemon/admin.c
> +++ b/daemon/admin.c
> @@ -37,6 +37,7 @@
>  #include "virnetserver.h"
>  #include "virstring.h"
>  #include "virthreadjob.h"
> +#include "virtypedparam.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_ADMIN
>  
> @@ -133,4 +134,62 @@ adminConnectGetLibVersion(virNetDaemonPtr dmn ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +static int
> +adminDispatchServerGetThreadpoolParameters(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                           virNetServerClientPtr client,
> +                                           virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                                           virNetMessageErrorPtr rerr,
> +                                           struct admin_server_get_threadpool_parameters_args *args,
> +                                           struct admin_server_get_threadpool_parameters_ret *ret)
> +{
> +    int rv = -1;
> +    virNetServerPtr srv = NULL;
> +    virTypedParameterPtr params = NULL;
> +    int nparams = 0;
> +    struct daemonAdmClientPrivate *priv =
> +        virNetServerClientGetPrivateData(client);
> +
> +    if (!(srv = virNetDaemonGetServer(priv->dmn, args->server.name))) {
> +        virReportError(VIR_ERR_NO_SERVER,
> +                       _("no server with matching name '%s' found"),
> +                       args->server.name);

There's no need for this virReportError(). virnetDaemonGetServer()
already reports an error.

> +        goto cleanup;
> +    }
> +
> +    if (adminDaemonGetThreadPoolParameters(srv, &params, &nparams,
> +                                           args->flags) < 0)
> +        goto cleanup;
> +
> +    if (nparams > ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Number of threadpool parameters %d exceeds max "
> +                         "allowed limit: %d"), nparams,
> +                       ADMIN_SERVER_THREADPOOL_PARAMETERS_MAX);
> +        goto cleanup;
> +    }
> +
> +    if (nparams) {
> +        if (VIR_ALLOC_N(ret->params.params_val, nparams) < 0)
> +            goto cleanup;

This will be leaked as virTypedParamsSerialize() allocates the array yet
again.

> +
> +        ret->params.params_len = nparams;
> +
> +        if (virTypedParamsSerialize(params, nparams,
> +                                    (virTypedParameterRemotePtr *) &ret->params.params_val,
> +                                    &ret->params.params_len, 0) < 0)
> +            goto cleanup;
> +    } else {
> +        ret->params.params_len = 0;
> +        ret->params.params_val = NULL;
> +    }

I think this is not necessary - virTypedParamsSerialize should handle
this case too.

> +
> +    rv = 0;
> + cleanup:
> +    if (rv < 0)
> +        virNetMessageSaveError(rerr);
> +
> +    virTypedParamsFree(params, nparams);
> +    virObjectUnref(srv);
> +    return rv;
> +}
>  #include "admin_dispatch.h"
> diff --git a/daemon/admin_server.c b/daemon/admin_server.c
> index 1d30ea5..00a310b 100644
> --- a/daemon/admin_server.c
> +++ b/daemon/admin_server.c
> @@ -31,6 +31,7 @@
>  #include "virnetdaemon.h"
>  #include "virnetserver.h"
>  #include "virstring.h"
> +#include "virthreadpool.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_ADMIN
>  
> @@ -68,3 +69,71 @@ adminConnectLookupServer(virNetDaemonPtr dmn,
>  
>      return virNetDaemonGetServer(dmn, name);
>  }
> +
> +int
> +adminDaemonGetThreadPoolParameters(virNetServerPtr srv,
> +                                   virTypedParameterPtr *params,
> +                                   int *nparams,
> +                                   unsigned int flags)
> +{
> +    int ret = -1;
> +    int maxparams = 0;
> +    size_t minWorkers;
> +    size_t maxWorkers;
> +    size_t nWorkers;
> +    size_t freeWorkers;
> +    size_t nPrioWorkers;
> +    size_t jobQueueDepth;
> +    virTypedParameterPtr tmpparams = NULL;
> +
> +    virCheckFlags(0, -1);
> +
> +    if (virNetServerGetThreadPoolParameters(srv, &minWorkers, &maxWorkers,
> +                                            &nWorkers, &freeWorkers,
> +                                            &nPrioWorkers,
> +                                            &jobQueueDepth) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to retrieve threadpool parameters"));
> +        goto cleanup;
> +    }
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_WORKERS_MIN,
> +                              minWorkers) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_WORKERS_MAX,
> +                              maxWorkers) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_WORKERS_CURRENT,
> +                              nWorkers) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_WORKERS_FREE,
> +                              freeWorkers) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_WORKERS_PRIORITY,
> +                              nPrioWorkers) < 0)
> +        goto cleanup;
> +
> +    if (virTypedParamsAddUInt(&tmpparams, nparams,
> +                              &maxparams, VIR_THREADPOOL_JOB_QUEUE_DEPTH,
> +                              jobQueueDepth) < 0)
> +        goto cleanup;
> +
> +    *params = tmpparams;
> +    tmpparams = NULL;
> +    ret = 0;
> +
> + cleanup:
> +    if (tmpparams)
> +        virTypedParamsFree(tmpparams, *nparams);

This redundant check is redundant. virTypedParamsFree() is capable of
accepting NULL just fine.

> +
> +    return ret;
> +}

> diff --git a/src/rpc/virnetserver.h b/src/rpc/virnetserver.h
> index aa24440..41c440d 100644
> --- a/src/rpc/virnetserver.h
> +++ b/src/rpc/virnetserver.h
> @@ -89,4 +89,17 @@ int virNetServerStart(virNetServerPtr srv);
>  
>  const char *virNetServerGetName(virNetServerPtr srv);
>  
> +int
> +virNetServerGetThreadPoolParameters(virNetServerPtr srv,
> +                                    size_t *minWorkers,
> +                                    size_t *maxWorkers,
> +                                    size_t *nWorkers,
> +                                    size_t *freeWorkers,
> +                                    size_t *nPrioWorkers,
> +                                    size_t *jobQueueDepth);
> +

> +int virNetServerSetThreadPoolParameters(virNetServerPtr srv,
> +                                        long long int minWorkers,
> +                                        long long int maxWorkers,
> +                                        long long int prioWorkers);

This does not belong here. I'm surprised that compiler does not bother
complaining.

>  #endif /* __VIR_NET_SERVER_H__ */
> 

Michal