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

Re: [libvirt] [RFC PATCH 1/5] Qemu Monitor API entry point.



On Tue, Apr 13, 2010 at 02:36:46PM -0400, Chris Lalancette wrote:
> Signed-off-by: Chris Lalancette <clalance redhat com>
> ---
>  include/libvirt/Makefile.am    |    1 +
>  include/libvirt/libvirt_qemu.h |   30 ++++++++++++
>  src/Makefile.am                |   11 ++++-
>  src/driver.h                   |    5 ++
>  src/esx/esx_driver.c           |    1 +
>  src/internal.h                 |    1 +
>  src/libvirt_qemu.c             |   96 ++++++++++++++++++++++++++++++++++++++++
>  src/lxc/lxc_driver.c           |    1 +
>  src/opennebula/one_driver.c    |    1 +
>  src/openvz/openvz_driver.c     |    1 +
>  src/phyp/phyp_driver.c         |    1 +
>  src/qemu/qemu_driver.c         |    1 +
>  src/remote/remote_driver.c     |    1 +
>  src/test/test_driver.c         |    1 +
>  src/uml/uml_driver.c           |    1 +
>  src/vbox/vbox_tmpl.c           |    1 +
>  src/xen/xen_driver.c           |    1 +
>  src/xenapi/xenapi_driver.c     |    1 +
>  18 files changed, 155 insertions(+), 1 deletions(-)
>  create mode 100644 include/libvirt/libvirt_qemu.h
>  create mode 100644 src/libvirt_qemu.c
> 
> diff --git a/include/libvirt/Makefile.am b/include/libvirt/Makefile.am
> index 8589dc5..ac5f32c 100644
> --- a/include/libvirt/Makefile.am
> +++ b/include/libvirt/Makefile.am
> @@ -3,6 +3,7 @@
>  virincdir = $(includedir)/libvirt
>  
>  virinc_HEADERS = libvirt.h		\
> +		 libvirt_qemu.h		\
>  		 virterror.h

Minor nitpick - I'd prefer an '-' instead of '_'

> diff --git a/include/libvirt/libvirt_qemu.h b/include/libvirt/libvirt_qemu.h
> new file mode 100644
> index 0000000..7d78a7f
> --- /dev/null
> +++ b/include/libvirt/libvirt_qemu.h
> @@ -0,0 +1,30 @@
> +/* -*- c -*-
> + * libvirt_qemu.h:
> + * Summary: qemu specific interfaces
> + * Description: Provides the interfaces of the libvirt library to handle
> + *              qemu specific methods
> + *
> + * Copy:  Copyright (C) 2010 Red Hat, Inc.
> + *
> + * See COPYING.LIB for the License of this software
> + *
> + * Author: Chris Lalancette <clalance redhat com>
> + */
> +
> +#ifndef __VIR_QEMU_H__
> +#define __VIR_QEMU_H__
> +
> +#include "libvirt.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +int virQemuMonitorCommand(virDomainPtr domain, const char *cmd, char **result,
> +                          unsigned int flags);

For naming, we should continue to keep the object name as the prefix, since
this makes things a little easier for python bindings. eg

   virDomainQemuMonitorRunCommand


> diff --git a/src/Makefile.am b/src/Makefile.am
> index d54e6d0..c01c94e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -32,7 +32,7 @@ if WITH_NETWORK
>  UUID=$(shell uuidgen 2>/dev/null)
>  endif
>  
> -lib_LTLIBRARIES = libvirt.la
> +lib_LTLIBRARIES = libvirt.la libvirt_qemu.la

And a '-' here too :-)

>  
>  moddir = $(libdir)/libvirt/drivers
>  mod_LTLIBRARIES =
> @@ -945,6 +945,15 @@ libvirt_test_la_LIBADD = $(libvirt_la_LIBADD)
>  libvirt_test_la_LDFLAGS = $(test_LDFLAGS)
>  libvirt_test_la_CFLAGS = $(COVERAGE_CFLAGS)
>  
> +libvirt_qemu_la_SOURCES = libvirt_qemu.c util/threads.c util/threads.h \
> +                               util/threads-pthread.h                   \
> +                               util/threads-win32.h                     \
> +                               util/virterror.c                         \
> +                               util/virterror_internal.h

Do we need to directly recompile those pieces here ? I think
if you just add 'libvirt.la' to the libvirt_qemu_la_LIBADD
it should be able to link to the existing built functions 

> +
> +libvirt_qemu_la_LDFLAGS = $(libvirt_la_LDFALGS)
> +libvirt_qemu_la_CFLAGS = $(COVERAGE_CFLAGS)
> +
>  
>  libexec_PROGRAMS =
>  
> diff --git a/src/driver.h b/src/driver.h
> index f8db9c1..651653d 100644
> --- a/src/driver.h
> +++ b/src/driver.h
> @@ -448,6 +448,10 @@ typedef int
>      (*virDrvDomainSnapshotDelete)(virDomainSnapshotPtr snapshot,
>                                    unsigned int flags);
>  
> +typedef int
> +    (*virDrvQemuMonitorCommand)(virDomainPtr domain, const char *cmd,
> +                                char **result, unsigned int flags);
> +
>  
>  /**
>   * _virDriver:
> @@ -558,6 +562,7 @@ struct _virDriver {
>      virDrvDomainSnapshotCurrent domainSnapshotCurrent;
>      virDrvDomainRevertToSnapshot domainRevertToSnapshot;
>      virDrvDomainSnapshotDelete domainSnapshotDelete;
> +    virDrvQemuMonitorCommand qemuMonitorCommand;
>  };

As mentioned in my reply to your question in patch 0, I think we
could possibly ignore the drivers here, and just call directly to
the QEMU function ?

> diff --git a/src/internal.h b/src/internal.h
> index 2e73210..57dc660 100644
> --- a/src/internal.h
> +++ b/src/internal.h
> @@ -22,6 +22,7 @@
>  # include "gettext.h"
>  
>  # include "libvirt/libvirt.h"
> +# include "libvirt/libvirt_qemu.h"
>  # include "libvirt/virterror.h"

It'd be better to avoid that here if possible - just have it included
in libvirt_qemu.c so we avoid polluting all the drivers with the QEMU
specific header

>  # include "libvirt_internal.h"
> diff --git a/src/libvirt_qemu.c b/src/libvirt_qemu.c
> new file mode 100644
> index 0000000..9c72fe2
> --- /dev/null
> +++ b/src/libvirt_qemu.c
> @@ -0,0 +1,96 @@
> +#include <config.h>
> +
> +#include "virterror_internal.h"
> +#include "logging.h"
> +#include "datatypes.h"
> +#include "driver.h"
> +
> +/**
> + * virLibConnError:
> + * @conn: the connection if available
> + * @error: the error number
> + * @info: extra information string
> + *
> + * Handle an error at the connection level
> + */
> +static void
> +virLibConnError(virConnectPtr conn, virErrorNumber error, const char *info)
> +{
> +    const char *errmsg;
> +
> +    if (error == VIR_ERR_OK)
> +        return;
> +
> +    errmsg = virErrorMsg(error, info);
> +    virRaiseError(conn, NULL, NULL, VIR_FROM_NONE, error, VIR_ERR_ERROR,
> +                  errmsg, info, NULL, 0, 0, errmsg, info);
> +}
> +
> +/**
> + * virLibDomainError:
> + * @domain: the domain if available
> + * @error: the error number
> + * @info: extra information string
> + *
> + * Handle an error at the connection level
> + */
> +static void
> +virLibDomainError(virDomainPtr domain, virErrorNumber error,
> +                  const char *info)
> +{
> +    virConnectPtr conn = NULL;
> +    const char *errmsg;
> +
> +    if (error == VIR_ERR_OK)
> +        return;
> +
> +    errmsg = virErrorMsg(error, info);
> +    if (error != VIR_ERR_INVALID_DOMAIN) {
> +        conn = domain->conn;
> +    }
> +    virRaiseError(conn, domain, NULL, VIR_FROM_DOM, error, VIR_ERR_ERROR,
> +                  errmsg, info, NULL, 0, 0, errmsg, info);
> +}
> +
> +int
> +virQemuMonitorCommand(virDomainPtr domain, const char *cmd, char **result,
> +                      unsigned int flags)
> +{
> +    virConnectPtr conn;
> +
> +    DEBUG("domain=%p, cmd=%s, result=%p, flags=%u", domain, cmd, result, flags);
> +
> +    virResetLastError();
> +
> +    if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> +        virLibDomainError(NULL, VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> +        virDispatchError(NULL);
> +        return -1;
> +    }
> +
> +    conn = domain->conn;
> +
> +    if (result == NULL) {
> +        virLibDomainError(domain, VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (conn->flags & VIR_CONNECT_RO) {
> +        virLibDomainError(domain, VIR_ERR_OPERATION_DENIED, __FUNCTION__);
> +        goto error;
> +    }
> +
> +    if (conn->driver->qemuMonitorCommand) {
> +        int ret;
> +        ret = conn->driver->qemuMonitorCommand(domain, cmd, result, flags);
> +        if (ret < 0)
> +            goto error;
> +        return ret;
> +    }
> +
> +    virLibConnError (conn, VIR_ERR_NO_SUPPORT, __FUNCTION__);
> +
> +error:
> +    virDispatchError(conn);
> +    return -1;
> +}



Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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