[libvirt] [PATCH v4 6/7] admin: Introduce adminDaemonConnectListServers API

[Michal Privoznik]

On 12.02.2016 11:08, Erik Skultety wrote:
> This API is merely a convenience API, i.e. when managing clients connected to
> daemon's servers, we should know (convenience) which server the specific client
> is connected to. This implies a client-side representation of a server along
> with a basic API to let the administrating client know what servers are actually
> available on the daemon.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  daemon/Makefile.am              |  2 +-
>  daemon/admin.c                  | 55 ++++++++++++++++++++++++++++
>  daemon/admin_server.c           | 73 +++++++++++++++++++++++++++++++++++++
>  daemon/admin_server.h           | 34 ++++++++++++++++++
>  include/libvirt/libvirt-admin.h |  8 ++++-
>  po/POTFILES.in                  |  1 +
>  src/admin/admin_protocol.x      | 18 +++++++++-
>  src/admin/admin_remote.c        | 71 ++++++++++++++++++++++++++++++++++++
>  src/admin_protocol-structs      | 12 +++++++
>  src/libvirt-admin.c             | 79 +++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_admin_private.syms  |  2 ++
>  src/libvirt_admin_public.syms   |  3 ++
>  src/rpc/virnetdaemon.c          | 39 ++++++++++++++++++++
>  src/rpc/virnetdaemon.h          |  1 +
>  14 files changed, 395 insertions(+), 3 deletions(-)
>  create mode 100644 daemon/admin_server.c
>  create mode 100644 daemon/admin_server.h
> 
> diff --git a/daemon/Makefile.am b/daemon/Makefile.am
> index 9edaf5f..2dbe81b 100644
> --- a/daemon/Makefile.am
> +++ b/daemon/Makefile.am
> @@ -128,7 +128,7 @@ libvirtd_conf_la_LIBADD = $(LIBXML_LIBS)
>  
>  noinst_LTLIBRARIES += libvirtd_admin.la
>  libvirtd_admin_la_SOURCES = \
> -		admin.c admin.h
> +		admin.c admin.h admin_server.c admin_server.h
>  
>  libvirtd_admin_la_CFLAGS = \
>  		$(AM_CFLAGS)		\
> diff --git a/daemon/admin.c b/daemon/admin.c
> index fa6caf3..0c1ddc0 100644
> --- a/daemon/admin.c
> +++ b/daemon/admin.c
> @@ -28,6 +28,7 @@
>  
>  #include "admin_protocol.h"
>  #include "admin.h"
> +#include "admin_server.h"
>  #include "datatypes.h"
>  #include "viralloc.h"
>  #include "virerror.h"
> @@ -77,6 +78,15 @@ remoteAdmClientInitHook(virNetServerClientPtr client ATTRIBUTE_UNUSED,
>      return priv;
>  }
>  
> +/* Helpers */
> +
> +static void
> +make_nonnull_server(admin_nonnull_server *srv_dst,
> +                    virAdmServerPtr srv_src)
> +{
> +    ignore_value(VIR_STRDUP_QUIET(srv_dst->name, srv_src->name));
> +}
> +
>  /* Functions */
>  static int
>  adminDispatchConnectOpen(virNetServerPtr server ATTRIBUTE_UNUSED,
> @@ -123,4 +133,49 @@ adminConnectGetLibVersion(virNetDaemonPtr dmn ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +static int
> +adminDispatchConnectListServers(virNetServerPtr server ATTRIBUTE_UNUSED,
> +                                virNetServerClientPtr client,
> +                                virNetMessagePtr msg ATTRIBUTE_UNUSED,
> +                                virNetMessageErrorPtr rerr ATTRIBUTE_UNUSED,
> +                                admin_connect_list_servers_args *args,
> +                                admin_connect_list_servers_ret *ret)
> +{
> +    virAdmServerPtr *servers = NULL;
> +    int nservers = 0;
> +    int rv = -1;
> +    size_t i;
> +    struct daemonAdmClientPrivate *priv =
> +        virNetServerClientGetPrivateData(client);
> +
> +    if ((nservers =
> +            adminDaemonListServers(priv->dmn,
> +                                   args->need_results ? &servers : NULL,
> +                                   args->flags)) < 0)
> +        goto cleanup;
> +
> +    if (servers && nservers) {
> +        if (VIR_ALLOC_N(ret->servers.servers_val, nservers) < 0)
> +            goto cleanup;
> +
> +        ret->servers.servers_len = nservers;
> +        for (i = 0; i < nservers; i++)
> +            make_nonnull_server(ret->servers.servers_val + i, servers[i]);
> +    } else {
> +        ret->servers.servers_len = 0;
> +        ret->servers.servers_val = NULL;
> +    }
> +
> +    ret->ret = nservers;
> +    rv = 0;
> +
> + cleanup:
> +    if (rv < 0)
> +        virNetMessageSaveError(rerr);
> +    if (servers && nservers > 0)
> +        for (i = 0; i < nservers; i++)
> +            virObjectUnref(servers[i]);
> +    VIR_FREE(servers);
> +    return rv;
> +}
>  #include "admin_dispatch.h"
> diff --git a/daemon/admin_server.c b/daemon/admin_server.c
> new file mode 100644
> index 0000000..a054421
> --- /dev/null
> +++ b/daemon/admin_server.c
> @@ -0,0 +1,73 @@
> +/*
> + * admin_server.c: admin methods to manage daemons and clients
> + *
> + * Copyright (C) 2016 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Authors: Erik Skultety <eskultet at redhat.com>
> + *          Martin Kletzander <mkletzan at redhat.com>
> + */
> +
> +#include <config.h>
> +
> +#include "admin_server.h"
> +#include "datatypes.h"
> +#include "viralloc.h"
> +#include "virerror.h"
> +#include "virlog.h"
> +#include "virnetdaemon.h"
> +#include "virnetserver.h"
> +#include "virstring.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_ADMIN
> +
> +VIR_LOG_INIT("daemon.admin_server");
> +
> +int
> +adminDaemonListServers(virNetDaemonPtr dmn,
> +                       virAdmServerPtr **servers,
> +                       unsigned int flags)
> +{
> +    int ret = -1;
> +    const char **srv_names = NULL;
> +    virAdmServerPtr *srvs = NULL;
> +    size_t i;
> +    ssize_t nsrvs = 0;
> +
> +    virCheckFlags(0, -1);
> +
> +    if ((nsrvs = virNetDaemonGetServerNames(dmn, &srv_names)) < 0)
> +        goto cleanup;
> +
> +    if (servers) {
> +        if (VIR_ALLOC_N(srvs, nsrvs + 1) < 0)

Here ^^ and below you are allocating just one more over what's needed.
So while you return the array size, you want it to be NULL terminated
too? Is that to be super safe?

> +            goto cleanup;
> +
> +        for (i = 0; i < nsrvs; i++) {
> +            if (!(srvs[i] = virAdmGetServer(NULL, srv_names[i])))
> +                goto cleanup;
> +        }
> +
> +        *servers = srvs;
> +        srvs = NULL;
> +    }
> +
> +    ret = nsrvs;
> +
> + cleanup:
> +    virObjectListFree(srvs);
> +    return ret;
> +}

Tentative ACK meanwhile.

Michal