[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 09/15] Add libvirt-admin library



On Thu, Apr 16, 2015 at 04:46:44PM +0200, Martin Kletzander wrote:
> Initial scratch of the admin library.  It has its own virAdmConnectPtr
> that inherits from virAbstractConnectPtr and thus trivially supports
> error reporting.

See my note earlier about error reporting on the connection being
a bad idea due to lack of thread safety.

> Configuration option --with-admin is added to control whether the admin
> library should be built or not (set to 'yes' by default).

Is there a compelling reason why we'd need/want to be able to
disable building of the admin library ?  As a comparison we
unconditionally build  libvirt-qemu.so & libvirt-lxc.so


> diff --git a/include/libvirt/libvirt-admin.h b/include/libvirt/libvirt-admin.h
> new file mode 100644
> index 0000000..7fe03cf
> --- /dev/null
> +++ b/include/libvirt/libvirt-admin.h
> @@ -0,0 +1,62 @@
> +/*
> + * libvirt-admin.h: Admin interface for libvirt
> + * Summary: Interfaces for handling server-related tasks
> + * Description: Provides the interfaces of the libvirt library to operate
> + *              with the server itself, not any hypervisors.
> + *
> + * Copyright (C) 2014-2015 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/>.
> + *
> + * Author: Martin Kletzander <mkletzan redhat com>
> + */
> +
> +#ifndef __VIR_ADMIN_H__
> +# define __VIR_ADMIN_H__
> +
> +# include "internal.h"
> +
> +# ifdef __cplusplus
> +extern "C" {
> +# endif
> +
> +
> +/**
> + * virAdmConnect:
> + *
> + * a virAdmConnect is a private structure representing a connection to
> + * libvirt daemon.
> + */
> +typedef struct _virAdmConnect virAdmConnect;
> +
> +/**
> + * virAdmConnectPtr:
> + *
> + * a virAdmConnectPtr is pointer to a virAdmConnect private structure,
> + * this is the type used to reference a connection to the daemon
> + * in the API.
> + */
> +typedef virAdmConnect *virAdmConnectPtr;
> +
> +virAdmConnectPtr virAdmConnectOpen(unsigned int flags);

How does this figure out which libvirtd daemon to connect to ? Presumably
you've hardcoded it based on the UID you're running as ?  I think for
future proofing we should probably define a URI syntax for this.

eg

   admin:///system
   admin:///session

And allow an optional parameter for the socket path, for people who
have built their daemon with an unusual --prefix arg.


> +libvirt_admin_la_SOURCES += 			\
> +		datatypes.c			\
> +		util/viralloc.c			\
> +		util/viratomic.c		\
> +		util/virauth.c			\
> +		util/virauthconfig.c		\
> +		util/virbitmap.c		\
> +		util/virbuffer.c		\
> +		util/vircommand.c		\
> +		util/virerror.c			\
> +		util/virevent.c			\
> +		util/vireventpoll.c		\
> +		util/virfile.c			\
> +		util/virhash.c			\
> +		util/virhashcode.c		\
> +		util/virjson.c			\
> +		util/virkeyfile.c		\
> +		util/virlog.c			\
> +		util/virobject.c		\
> +		util/virpidfile.c		\
> +		util/virprocess.c		\
> +		util/virrandom.c		\
> +		util/virseclabel.c		\
> +		util/virsocketaddr.c		\
> +		util/virstorageencryption.c	\
> +		util/virstoragefile.c		\
> +		util/virstring.c		\
> +		util/virthread.c		\
> +		util/virtime.c			\
> +		util/virtypedparam.c		\
> +		util/viruri.c			\
> +		util/virutil.c			\
> +		util/viruuid.c			\
> +		util/virxml.c			\
> +		remote/remote_protocol.c	\
> +		rpc/virnetmessage.h		\
> +		rpc/virnetmessage.c		\
> +		rpc/virnetsocket.c		\
> +		rpc/virnetsshsession.c		\
> +		rpc/virkeepalive.c		\
> +		rpc/virnetclient.c		\
> +		rpc/virnetclientprogram.c	\
> +		rpc/virnetclientstream.c	\
> +		rpc/virnetprotocol.c		\
> +		rpc/virnettlscontext.c		\
> +		rpc/virnetsaslcontext.c



> +
> +libvirt_admin_la_LDFLAGS = \
> +		$(VERSION_SCRIPT_FLAGS)$(LIBVIRT_ADMIN_SYMBOL_FILE)	\
> +		-version-info $(LIBVIRT_VERSION_INFO)			\
> +		$(AM_LDFLAGS) 						\
> +		$(CYGWIN_EXTRA_LDFLAGS) 				\
> +		$(MINGW_EXTRA_LDFLAGS)
> +
> +libvirt_admin_la_LIBADD = \
> +		$(CYGWIN_EXTRA_LIBADD)
> +
> +libvirt_admin_la_CFLAGS = \
> +		$(AM_CFLAGS)		\
> +		-I$(srcdir)/remote	\
> +		-I$(srcdir)/rpc		\
> +		-I$(srcdir)/admin
> +
> +libvirt_admin_la_CFLAGS += \
> +		$(CAPNG_CFLAGS)			\
> +		$(YAJL_CFLAGS)			\
> +		$(SSH2_CFLAGS)			\
> +		$(SASL_CFLAGS)			\
> +		$(GNUTLS_CFLAGS)
> +
> +libvirt_admin_la_LIBADD += \
> +		$(CAPNG_LIBS)			\
> +		$(YAJL_LIBS)			\
> +		$(DEVMAPPER_LIBS)		\
> +		$(LIBXML_LIBS)			\
> +		$(SSH2_LIBS)			\
> +		$(SASL_LIBS)			\
> +		$(GNUTLS_LIBS)
> +
> +if WITH_DTRACE_PROBES
> +libvirt_admin_la_LIBADD += libvirt_probes.lo
> +endif WITH_DTRACE_PROBES
> +
> +endif WITH_ADMIN
> +
>  # Empty source list - it merely links a bunch of convenience libs together
>  libvirt_la_SOURCES =
>  libvirt_la_LDFLAGS = \
> diff --git a/src/datatypes.c b/src/datatypes.c
> index b21113e..83cee7e 100644
> --- a/src/datatypes.c
> +++ b/src/datatypes.c
> @@ -59,6 +59,10 @@ static void virStreamDispose(void *obj);
>  static void virStorageVolDispose(void *obj);
>  static void virStoragePoolDispose(void *obj);
> 
> +virClassPtr virAdmConnectClass;
> +
> +static void virAdmConnectDispose(void *obj);
> +
>  static int
>  virDataTypesOnceInit(void)
>  {
> @@ -88,6 +92,8 @@ virDataTypesOnceInit(void)
>      DECLARE_CLASS(virStorageVol);
>      DECLARE_CLASS(virStoragePool);
> 
> +    DECLARE_CLASS_CONNECT(virAdmConnect);
> +
>  #undef DECLARE_CLASS_COMMON
>  #undef DECLARE_CLASS_LOCKABLE
>  #undef DECLARE_CLASS_CONNECT
> @@ -804,3 +810,27 @@ virDomainSnapshotDispose(void *obj)
>      VIR_FREE(snapshot->name);
>      virObjectUnref(snapshot->domain);
>  }
> +
> +
> +virAdmConnectPtr
> +virAdmConnectNew(void)
> +{
> +    virAdmConnectPtr ret;
> +
> +    if (virDataTypesInitialize() < 0)
> +        return NULL;
> +
> +    if (!(ret = virGetAbstractConnect(virAdmConnectClass)))
> +        return NULL;
> +
> +    return ret;
> +}
> +
> +static void
> +virAdmConnectDispose(void *obj)
> +{
> +    virAdmConnectPtr conn = obj;
> +
> +    if (conn->privateDataFreeFunc)
> +        conn->privateDataFreeFunc(conn->privateData);
> +}
> diff --git a/src/datatypes.h b/src/datatypes.h
> index 9f95811..b240e4c 100644
> --- a/src/datatypes.h
> +++ b/src/datatypes.h
> @@ -42,6 +42,8 @@ extern virClassPtr virStreamClass;
>  extern virClassPtr virStorageVolClass;
>  extern virClassPtr virStoragePoolClass;
> 
> +extern virClassPtr virAdmConnectClass;
> +
>  # define virCheckConnectReturn(obj, retval)                             \
>      do {                                                                \
>          if (!virObjectIsClass(obj, virConnectClass)) {                  \
> @@ -296,6 +298,26 @@ extern virClassPtr virStoragePoolClass;
>                    dom, NULLSTR(_domname), _uuidstr, __VA_ARGS__);       \
>      } while (0)
> 
> +# define virCheckAdmConnectReturn(obj, retval)                          \
> +    do {                                                                \
> +        if (!virObjectIsClass(obj, virAdmConnectClass)) {               \
> +            virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INVALID_CONN,   \
> +                                 __FILE__, __FUNCTION__, __LINE__,      \
> +                                 __FUNCTION__);                         \
> +            virDispatchError(NULL);                                     \
> +            return retval;                                              \
> +        }                                                               \
> +    } while (0)
> +# define virCheckAdmConnectGoto(obj, label)                             \
> +    do {                                                                \
> +        if (!virObjectIsClass(obj, virAdmConnectClass)) {               \
> +            virReportErrorHelper(VIR_FROM_THIS, VIR_ERR_INVALID_CONN,   \
> +                                 __FILE__, __FUNCTION__, __LINE__,      \
> +                                 __FUNCTION__);                         \
> +            goto label;                                                 \
> +        }                                                               \
> +    } while (0)
> +
>  /**
>   * VIR_DOMAIN_DEBUG:
>   * @dom: domain
> @@ -365,6 +387,19 @@ struct _virConnect {
>  };
> 
>  /**
> + * _virAdmConnect:
> + *
> + * Internal structure associated to an admin connection
> + */
> +struct _virAdmConnect {
> +    virAbstractConnect object;
> +
> +    void *privateData;
> +    virFreeCallback privateDataFreeFunc;
> +};
> +
> +
> +/**
>  * _virDomain:
>  *
>  * Internal structure associated to a domain
> @@ -545,4 +580,6 @@ virNWFilterPtr virGetNWFilter(virConnectPtr conn,
>  virDomainSnapshotPtr virGetDomainSnapshot(virDomainPtr domain,
>                                            const char *name);
> 
> +virAdmConnectPtr virAdmConnectNew(void);
> +
>  #endif /* __VIR_DATATYPES_H__ */
> diff --git a/src/internal.h b/src/internal.h
> index 4d473af..1fbcfc2 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -58,6 +58,7 @@
>  # include "libvirt/libvirt.h"
>  # include "libvirt/libvirt-lxc.h"
>  # include "libvirt/libvirt-qemu.h"
> +# include "libvirt/libvirt-admin.h"
>  # include "libvirt/virterror.h"
> 
>  # include "c-strcase.h"
> diff --git a/src/libvirt-admin.c b/src/libvirt-admin.c
> new file mode 100644
> index 0000000..e47089d
> --- /dev/null
> +++ b/src/libvirt-admin.c
> @@ -0,0 +1,337 @@
> +/*
> + * libvirt-admin.c
> + *
> + * Copyright (C) 2014-2015 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/>.
> + *
> + * Author: Martin Kletzander <mkletzan redhat com>
> + */
> +
> +#include <config.h>
> +
> +#include <rpc/rpc.h>
> +
> +#include "internal.h"
> +#include "configmake.h"
> +#include "datatypes.h"
> +#include "viralloc.h"
> +#include "virlog.h"
> +#include "virstring.h"
> +#include "virutil.h"
> +#include "viruuid.h"
> +#include "virnetclient.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_ADMIN
> +#define LIBVIRTD_ADMIN_UNIX_SOCKET LOCALSTATEDIR "/run/libvirt/libvirt-admin-sock"
> +
> +VIR_LOG_INIT("libvirt-admin");
> +
> +
> +typedef struct _remoteAdminPriv remoteAdminPriv;
> +typedef remoteAdminPriv *remoteAdminPrivPtr;
> +
> +struct _remoteAdminPriv {
> +    virMutex lock;
> +    int counter;
> +    int localUses;
> +    virNetClientPtr client;
> +    virNetClientProgramPtr program;
> +};
> +
> +static void
> +remoteAdminLock(remoteAdminPrivPtr priv)
> +{
> +    virMutexLock(&priv->lock);
> +}
> +
> +static void
> +remoteAdminUnlock(remoteAdminPrivPtr priv)
> +{
> +    virMutexUnlock(&priv->lock);
> +}
> +
> +static void
> +remoteAdminPrivFree(void *opaque)
> +{
> +    remoteAdminPrivPtr priv = opaque;
> +
> +    virObjectUnref(priv->program);
> +    virObjectUnref(priv->client);
> +
> +    virMutexDestroy(&priv->lock);
> +    VIR_FREE(priv);
> +}
> +
> +static int
> +callFull(virAdmConnectPtr conn ATTRIBUTE_UNUSED,
> +         remoteAdminPrivPtr priv,
> +         int *fdin,
> +         size_t fdinlen,
> +         int **fdout,
> +         size_t *fdoutlen,
> +         int proc_nr,
> +         xdrproc_t args_filter, char *args,
> +         xdrproc_t ret_filter, char *ret)
> +{
> +    int rv;
> +    virNetClientProgramPtr prog;
> +    int counter = priv->counter++;
> +    virNetClientPtr client = priv->client;
> +    priv->localUses++;
> +
> +    /* We hav nothing else right now, but we can still add more
> +     * programs in the future */
> +    prog = priv->program;
> +
> +    /* Unlock, so that if we get any async events/stream data
> +     * while processing the RPC, we don't deadlock when our
> +     * callbacks for those are invoked
> +     */
> +    remoteAdminUnlock(priv);
> +    rv = virNetClientProgramCall(prog,
> +                                 client,
> +                                 counter,
> +                                 proc_nr,
> +                                 fdinlen, fdin,
> +                                 fdoutlen, fdout,
> +                                 args_filter, args,
> +                                 ret_filter, ret);
> +    remoteAdminLock(priv);
> +    priv->localUses--;
> +
> +    return rv;
> +}
> +
> +static int
> +call(virAdmConnectPtr conn,
> +     unsigned int flags,
> +     int proc_nr,
> +     xdrproc_t args_filter, char *args,
> +     xdrproc_t ret_filter, char *ret)
> +{
> +    virCheckFlags(0, -1);
> +
> +    return callFull(conn, conn->privateData,
> +                    NULL, 0, NULL, NULL, proc_nr,
> +                    args_filter, args, ret_filter, ret);
> +}
> +
> +#include "admin_protocol.h"
> +#include "admin_client.h"
> +
> +static bool virAdmGlobalError;
> +static virOnceControl virAdmGlobalOnce = VIR_ONCE_CONTROL_INITIALIZER;
> +
> +static remoteAdminPrivPtr
> +remoteAdminPrivNew(void)
> +{
> +    remoteAdminPrivPtr priv = NULL;
> +
> +    /* initialize private data */
> +    if (VIR_ALLOC(priv) < 0)
> +        goto error;
> +
> +    if (virMutexInit(&priv->lock) < 0) {
> +        VIR_FREE(priv);
> +        goto error;
> +    }
> +
> +    priv->localUses = 1;
> +    if (!(priv->client = virNetClientNewUNIX(LIBVIRTD_ADMIN_UNIX_SOCKET,
> +                                             false, NULL)))
> +        goto error;
> +
> +    if (!(priv->program = virNetClientProgramNew(ADMIN_PROGRAM,
> +                                                 ADMIN_PROTOCOL_VERSION,
> +                                                 NULL, 0, NULL)))
> +        goto error;
> +
> +    if (virNetClientAddProgram(priv->client, priv->program) < 0)
> +        goto error;
> +
> +    return priv;
> + error:
> +    remoteAdminPrivFree(priv);
> +    return NULL;
> +}
> +
> +static void
> +virAdmGlobalInit(void)
> +{
> +    /* It would be nice if we could trace the use of this call, to
> +     * help diagnose in log files if a user calls something other than
> +     * virAdmConnectOpen first.  But we can't rely on VIR_DEBUG working
> +     * until after initialization is complete, and since this is
> +     * one-shot, we never get here again.  */
> +    if (virThreadInitialize() < 0 ||
> +        virErrorInitialize() < 0)
> +        goto error;
> +
> +    virLogSetFromEnv();
> +
> +    if (!bindtextdomain(PACKAGE, LOCALEDIR))
> +        goto error;
> +
> +    return;
> + error:
> +    virAdmGlobalError = true;
> +}
> +
> +/**
> + * virAdmInitialize:
> + *
> + * Initialize the library.
> + *
> + * Returns 0 in case of success, -1 in case of error
> + */
> +static int
> +virAdmInitialize(void)
> +{
> +    if (virOnce(&virAdmGlobalOnce, virAdmGlobalInit) < 0)
> +        return -1;
> +
> +    if (virAdmGlobalError)
> +        return -1;
> +
> +    return 0;
> +}
> +
> +/**
> + * virAdmConnectOpen:
> + * @flags: unused, must be 0
> + *
> + * Opens connection to admin interface of the daemon.
> + *
> + * Returns @virAdmConnectPtr object or NULL on error
> + */
> +virAdmConnectPtr
> +virAdmConnectOpen(unsigned int flags)
> +{
> +    virAdmConnectPtr conn = NULL;
> +    remoteAdminPrivPtr priv = NULL;
> +
> +    if (virAdmInitialize() < 0)
> +        goto error;
> +
> +    VIR_DEBUG("flags=%x", flags);
> +    virResetLastError();
> +
> +    if (!(conn = virAdmConnectNew()))
> +        goto error;
> +
> +    if (!(priv = remoteAdminPrivNew()))
> +        goto error;
> +
> +    remoteAdminLock(priv);
> +    conn->privateData = priv;
> +    conn->privateDataFreeFunc = remoteAdminPrivFree;
> +
> +    {
> +        admin_connect_open_args args = { flags };
> +
> +        VIR_DEBUG("Trying to open admin connection");
> +        if (call(conn, 0, ADMIN_PROC_CONNECT_OPEN,
> +                 (xdrproc_t) xdr_admin_connect_open_args, (char *) &args,
> +                 (xdrproc_t) xdr_void, (char *) NULL) == -1)
> +            goto error;
> +    }
> +
> +    remoteAdminUnlock(priv);
> +
> +    return conn;
> + error:
> +    virDispatchError(NULL);
> +    if (priv)
> +        remoteAdminUnlock(priv);
> +    virObjectUnref(conn);
> +    return NULL;
> +}
> +
> +/**
> + * virAdmConnectClose:
> + * @conn: pointer to admin connection to close
> + *
> + * This function closes the connection to the Hypervisor. This should
> + * not be called if further interaction with the Hypervisor are needed
> + * especially if there is running domain which need further monitoring by
> + * the application.
> + *
> + * Connections are reference counted; the count is explicitly
> + * increased by the initial open (virConnectOpen, virConnectOpenAuth,
> + * and the like) as well as virConnectRef; it is also temporarily
> + * increased by other API that depend on the connection remaining
> + * alive.  The open and every virConnectRef call should have a
> + * matching virConnectClose, and all other references will be released
> + * after the corresponding operation completes.
> + *
> + * Returns a positive number if at least 1 reference remains on
> + * success. The returned value should not be assumed to be the total
> + * reference count. A return of 0 implies no references remain and
> + * the connection is closed and memory has been freed. A return of -1
> + * implies a failure.
> + *
> + * It is possible for the last virConnectClose to return a positive
> + * value if some other object still has a temporary reference to the
> + * connection, but the application should not try to further use a
> + * connection after the virConnectClose that matches the initial open.
> + */
> +int
> +virAdmConnectClose(virAdmConnectPtr conn)
> +{
> +    VIR_DEBUG("conn=%p", conn);
> +
> +    virResetLastError();
> +    if (!conn)
> +        return 0;
> +
> +    virCheckAdmConnectReturn(conn, -1);
> +
> +    if (!virObjectUnref(conn))
> +        return 0;
> +    return 1;
> +}
> +
> +
> +/**
> + * virAdmConnectRef:
> + * @conn: the connection to hold a reference on
> + *
> + * Increment the reference count on the connection. For each
> + * additional call to this method, there shall be a corresponding
> + * call to virConnectClose to release the reference count, once
> + * the caller no longer needs the reference to this object.
> + *
> + * This method is typically useful for applications where multiple
> + * threads are using a connection, and it is required that the
> + * connection remain open until all threads have finished using
> + * it. I.e., each new thread using a connection would increment
> + * the reference count.
> + *
> + * Returns 0 in case of success, -1 in case of failure
> + */
> +int
> +virAdmConnectRef(virAdmConnectPtr conn)
> +{
> +    VIR_DEBUG("conn=%p refs=%d", conn,
> +              conn ? conn->object.parent.parent.u.s.refs : 0);
> +
> +    virResetLastError();
> +    virCheckAdmConnectReturn(conn, -1);
> +
> +    virObjectRef(conn);
> +
> +    return 0;
> +}
> diff --git a/src/libvirt_admin.syms b/src/libvirt_admin.syms
> new file mode 100644
> index 0000000..292433f
> --- /dev/null
> +++ b/src/libvirt_admin.syms
> @@ -0,0 +1,18 @@
> +#
> +# Officially exported symbols, for which header
> +# file definitions are installed in /usr/include/libvirt
> +# from libvirt-admin.h
> +#
> +# Versions here are *fixed* to match the libvirt version
> +# at which the symbol was introduced. This ensures that
> +# a new client app requiring symbol foo() can't accidentally
> +# run with old libvirt-admin.so not providing foo() - the global
> +# soname version info can't enforce this since we never
> +# change the soname
> +#
> +LIBVIRT_ADMIN_1.2.3 {

Either you've been working on this a really long time, or this
version was a typo.

Either way, when we are ready to merge the admin library, that
might be a nice reason to bump our version to 1.3.x, since
we've been on 1.2.x a long time.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]