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

Re: [libvirt] [PATCH 1/4] Addition of XenAPI support to libvirt



On Fri, Feb 19, 2010 at 11:14:53AM +0000, Sharadha Prabhakar (3P) wrote:
> Resending patches in plain text. ~/Libvirt/Src/Makefile.am contains some changes suggested by
> Daniel Veillard.
> 
> This is a patch to add XenAPI driver support for libvirt version 0.7.6. XenAPI can be used against XenCloud platform and
> managed through virsh and virt-manger. This patch supports domain related APIs in libvirt. It is possible to get domain information,
> list active and inactive domains, get Domain XML configuration and Start/stop/pause/shutdown/destroy VMs.
> There will be more patches after this review to support more libvirt APIs and add remote storage support to XenAPI.
> In order to run this patch you would require libxenserver library.
> Libxenserver library can be downloaded from http://community.citrix.com/download/attachments/38633496/libxenserver-5.5.0-1-src.tar.bz2?version=1
> Copy the libxenserver folder in the same directory level as libvirt.
> The XenCloud platform can be downloaded from  http://xen.org/products/cloudxen.html
> 
> diff -ur ./libvirt_org/configure.ac ./libvirt/configure.ac
> --- ./libvirt_org/configure.ac  2010-02-17 17:39:21.000000000 +0000
> +++ ./libvirt/configure.ac      2010-02-18 11:51:29.000000000 +0000
> @@ -219,6 +219,8 @@
>    AC_HELP_STRING([--with-libssh2=@<:@PFX@:>@], [libssh2 location @<:@default=/usr/local/lib@:>@]),[],[with_libssh2=yes])
>  AC_ARG_WITH([phyp],
>    AC_HELP_STRING([--with-phyp], [add PHYP support @<:@default=check@:>@]),[],[with_phyp=check])
> +AC_ARG_WITH([xenapi],
> +  AC_HELP_STRING([--with-xenapi], [add XenAPI support @<:@default=yes@:>@]),[],[with_xenapi=check])
>  AC_ARG_WITH([vbox],
>    AC_HELP_STRING([--with-vbox], [add VirtualBox support @<:@default=yes@:>@]),[],[with_vbox=yes])
>  AC_ARG_WITH([lxc],
> @@ -307,6 +309,11 @@
>  fi
>  AM_CONDITIONAL([WITH_VBOX], [test "$with_vbox" = "yes"])
> 
> +if test "$with_xenapi" = "yes"; then
> +    AC_DEFINE_UNQUOTED([WITH_XENAPI], 1, [whether XenAPI driver is enabled])
> +fi
> +AM_CONDITIONAL([WITH_XENAPI], [test "$with_xenapi" = "yes"])

This should really be checking for the libxenserver library. I'd expect
the behaviour to be

 --with-xenapi            -> check for the libxenserver in regular compiler/linker
                             paths (ie let it default to /usr/include & /usr/lib)
 --with-xenapi=/some/dir  -> set CFLAGS=-I/some/dir/include LDFLAGS=-L/some/dir/lib

once it is found then export two variables to the makefile

  LIBXENSERVER_CFLAGS
  LIBXENSERVER_LDFLAGS


> +
>  if test "$with_libvirtd" = "no" ; then
>    with_qemu=no
>  fi
> @@ -1894,6 +1901,7 @@
>  AC_MSG_NOTICE([     UML: $with_uml])
>  AC_MSG_NOTICE([  OpenVZ: $with_openvz])
>  AC_MSG_NOTICE([    VBox: $with_vbox])
> +AC_MSG_NOTICE([  XenAPI: $with_xenapi])
>  AC_MSG_NOTICE([     LXC: $with_lxc])
>  AC_MSG_NOTICE([    PHYP: $with_phyp])
>  AC_MSG_NOTICE([     ONE: $with_one])
> Binary files ./libvirt_org/daemon/.libs/libvirtd and ./libvirt/daemon/.libs/libvirtd differ
> Only in ./libvirt_org/daemon/.libs: lt-libvirtd
> 
> --- ./libvirt_org/src/Makefile.am       2010-02-17 17:38:13.000000000 +0000
> +++ ./libvirt/src/Makefile.am   2010-02-19 10:55:51.000000000 +0000
> @@ -3,12 +3,19 @@
>  # No libraries with the exception of LIBXML should be listed
>  # here. List them against the individual XXX_la_CFLAGS targets
>  # that actually use them
> +
> +XENAPI_CFLAGS = -I top_srcdir@/../libxenserver/include
> +

This is not required if variables are exported from the configure.ac script

>  INCLUDES =                                                     \
>                 -I$(top_srcdir)/gnulib/lib                      \
>                 -I../gnulib/lib                                 \
>                 -I../include                                    \
> +                -I/usr/include                                  \
>                 -I top_srcdir@/src/util                         \
> +               -I top_srcdir@/src                              \
> +               -I top_srcdir@/src/xenapi                       \
>                 -I top_srcdir@/include                          \
> +               $(XENAPI_CFLAGS)                                \
>                 $(DRIVER_MODULE_CFLAGS)                         \
>                 $(LIBXML_CFLAGS)                                \
>                 -DLIBDIR=\""$(libdir)"\"                        \

The global INCLUDES variable should not contain any driver specific
rules - this is only for stuff shared between drivers

> @@ -42,6 +49,9 @@
>  augeastestdir = $(datadir)/augeas/lenses/tests
>  augeastest_DATA =
> 
> +XENAPI_LIBS = @top_srcdir@/../libxenserver/libxenserver.so
> +XENAPI_LDFLAGS = -L top_srcdir@/../libxenserver/ -lxenserver

Also redundant if exported from configure.ac

> +
>  # These files are not related to driver APIs. Simply generic
>  # helper APIs for various purposes
>  UTIL_SOURCES =                                                 \
> @@ -205,6 +215,10 @@
>                 qemu/qemu_security_dac.h                        \
>                 qemu/qemu_security_dac.c
> 
> +XENAPI_DRIVER_SOURCES =                                         \
> +                xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
> +                xenapi/xenapi_utils.c xenapi/xenapi_utils.h
> +
>  UML_DRIVER_SOURCES =                                           \
>                 uml/uml_conf.c uml/uml_conf.h                   \
>                 uml/uml_driver.c uml/uml_driver.h
> @@ -466,6 +480,28 @@
>  libvirt_driver_vbox_la_SOURCES = $(VBOX_DRIVER_SOURCES)
>  endif
> 
> +if WITH_XENAPI
> +if WITH_DRIVER_MODULES
> +mod_LTLIBRARIES += libvirt_driver_xenapi.la
> +else
> +noinst_LTLIBRARIES += libvirt_driver_xenapi.la
> +
> +libvirt_la_LIBADD += libvirt_driver_xenapi.la   \
> +                     $(XENAPI_LIBS)
> +endif
> +#libvirt_driver_xenapi_la_LIBADD = $(XENAPI_LIBS)
> +libvirt_driver_xenapi_la_CFLAGS = $(XEN_CFLAGS) \
> +                                  $(LIBXML_CFLAGS) \
> +                                  $(shell curl-config --cflags)
> +libvirt_driver_xenapi_la_LDFLAGS = $(XENAPI_LDFLAGS) -g $(LIBXML_LIBS) \
> +                                   $(shell curl-config --libs)

I think you mean XENAPI_CFLAGS here, not XEN_CFLAGS. Also the
probing for curl should be done from configure.ac. Indeed it
already checks for libcurl, so you just need to reference the
existing LIBCURL_CFLAGS/LDFALGS. There's no need for LIBXML_CFLAGS
here, since that's in the global INCLUDES already

> +
> +if WITH_DRIVER_MODULES
> +libvirt_driver_xenapi_la_LDFLAGS += -module -avoid-version
> +endif
> +libvirt_driver_xenapi_la_SOURCES = $(XENAPI_DRIVER_SOURCES)
> +endif
> +
>  if WITH_QEMU
>  if WITH_DRIVER_MODULES
>  mod_LTLIBRARIES += libvirt_driver_qemu.la
> @@ -722,6 +758,7 @@
>                 $(OPENVZ_DRIVER_SOURCES)                        \
>                 $(PHYP_DRIVER_SOURCES)                          \
>                 $(VBOX_DRIVER_SOURCES)                          \
> +               $(XENAPI_DRIVER_SOURCES)                        \
>                 $(ESX_DRIVER_SOURCES)                           \
>                 $(NETWORK_DRIVER_SOURCES)                       \
>                 $(INTERFACE_DRIVER_SOURCES)                     \
> 
> diff -ur ./libvirt_org/tools/Makefile.am ./libvirt/tools/Makefile.am
> --- ./libvirt_org/tools/Makefile.am     2010-02-17 17:36:13.000000000 +0000
> +++ ./libvirt/tools/Makefile.am 2010-02-18 11:56:30.000000000 +0000
> @@ -40,7 +40,8 @@
>                 $(WARN_CFLAGS)                                  \
>                 ../src/libvirt.la                               \
>                 ../gnulib/lib/libgnu.la                         \
> -               $(VIRSH_LIBS)
> +               $(VIRSH_LIBS)                                   \
> +                ../../libxenserver/libxenserver.so
>  virsh_CFLAGS =                                                         \
>                 -I$(top_srcdir)/gnulib/lib -I../gnulib/lib      \
>                 -I../include -I$(top_srcdir)/include            \

This change should not be required - virsh links to the public libvirt
API only, never the stuff used by the internal drivers

Regards,
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]