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

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



Please see my comments inline. The diff content of PATCH 2/2 with the changes is
submitted at the end of this mail.

@<:@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])

>The actual default and the printed one mismatch. change default=yes to
>default=check

This has been changed.

>  AC_ARG_WITH([vbox],
>   AC_HELP_STRING([--with-vbox], [add VirtualBox support @<:@default=yes@:>@]),[],[with_vbox=yes])
>  AC_ARG_WITH([lxc],
> @@ -307,6 +309,7 @@
>  fi
>  AM_CONDITIONAL([WITH_VBOX], [test "$with_vbox" = "yes"])
>
> +
>  if test "$with_libvirtd" = "no" ; then
>   with_qemu=no
>  fi
> @@ -335,6 +338,49 @@
>  fi
>  AM_CONDITIONAL([WITH_LIBVIRTD], [test "$with_libvirtd" = "yes"])
>
> +
> +old_LIBS="$LIBS"
> +old_CFLAGS="$CFLAGS"
> +LIBXENSERVER_LIBS=""
> +LIBXENSERVER_CFLAGS=""
> +LIBXENSERVER_LDFLAGS=""
> +dnl search for the XenServer library
> +if test "$with_xenapi" != "no" ; then
> +    if test "$with_xenapi" != "yes" -a "$with_xenapi" != "check" ; then
> +        LIBXENSERVER_CFLAGS="-I$with_xenapi/include"
> +        LIBXENSERVER_LDFLAGS="$with_xenapi/libxenserver.so"

>Why do you add the path to an .so file to the LDFLAGS?
I wasn't sure that adding LIBXENSERVER_LIBS to xenapi_ldflags would
Suffice. LIBXENSERVER_LDFLAGS has been removed now.

> +        LIBXENSERVER_LIBS="-L$with_xenapi -lxenserver"
> +    fi
> +    fail=0
> +    CFLAGS="$CFLAGS $LIBXENSERVER_CFLAGS"
> +    LIBS="$LIBS $LIBXENSERVER_LIBS"
> +    AC_CHECK_LIB([xen_vm_start], [xs_read], [

>I'm not sure that I understand what you're doing here. You're checking
>for a xs_read function in a xen_vm_start library.

This has been changed to check for the function xen_vm_start in libxenserver.


> +           with_xenapi=yes
> +           LIBXENSERVER_LIBS="$LIBXENSERVER_LIBS -lxenstore"

>Do you really need xenstore?
No. It has been changed to -lxenserver


> +       ],[
> +           if test "$with_xenapi" = "yes"; then
> +               fail=1
> +           fi
> +           with_xenapi=no
> +       ])
> +fi
> +
> +LIBS="$old_LIBS"
> +CFLAGS="$old_CFLAGS"
> +
> +if test $fail = 1; then
> +    AC_MSG_ERROR([You must install the XenServer Library to compile XenAPI driver with -lxenserver])
> +fi
> +
> +if test "$with_xenapi" = "yes"; then
> +    AC_DEFINE_UNQUOTED([WITH_XENAPI], 1, [whether Xen driver is enabled])

>'XenAPI driver' instead of 'Xen driver'
Done

> +fi
> +
> +AC_SUBST([LIBXENSERVER_CFLAGS])
> +AC_SUBST([LIBXENSERVER_LIBS])
> +AC_SUBST([LIBXENSERVER_LDFLAGS])
> +
> +



>  old_LIBS="$LIBS"
>  old_CFLAGS="$CFLAGS"
>  XEN_LIBS=""
> @@ -1447,23 +1493,31 @@
>  LIBCURL_LIBS=""
>  LIBCURL_FOUND="no"
>
> -if test "$with_esx" = "yes" -o "$with_esx" = "check"; then
> +if test "$with_esx" = "yes" -o "$with_esx" = "check" -o "$with_xenapi" = "yes" -o "$with_xenapi" = "check"; >then
>     PKG_CHECK_MODULES(LIBCURL, libcurl >= $LIBCURL_REQUIRED, [
>         LIBCURL_FOUND=yes
>         with_esx="yes"
> +        with_xenapi="yes"

>This part should be split into an ESX and XenAPI part. Otherwise you
>may change with_esx="no" to with_esx="yes" just because libcurl was
>found.


>     ], [
> -        if test "$with_esx" = "check"; then
> +        if test "$with_esx" = "check" -o "$with_xenapi" = "check"; then
>             with_esx=no
> -            AC_MSG_NOTICE([libcurl is required for ESX driver, disabling it])
> +            with_xenapi=no
> +            AC_MSG_NOTICE([libcurl is required for ESX/XENAPI driver, disabling it])
>         else
> -            AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for the ESX driver])
> +            AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for the ESX/XENAPI driver])
>         fi

>Also this part should be split into an ESX and XenAPI part.

>     ])
>  fi
>  if test "$with_esx" = "yes" ; then
>     AC_DEFINE_UNQUOTED([WITH_ESX], 1, [whether ESX driver is enabled])
>  fi
> +
> +if test "$with_xenapi" = "yes" ; then
> +    AC_DEFINE_UNQUOTED([WITH_XENAPI], 1, [whether XenAPI driver is enabled])
> +fi
> +
>  AM_CONDITIONAL([WITH_ESX], [test "$with_esx" = "yes"])
> +AM_CONDITIONAL([WITH_XENAPI], [test "$with_xenapi" = "yes"])
>  AC_SUBST([LIBCURL_CFLAGS])
>  AC_SUBST([LIBCURL_LIBS])

>I suggest to restructure this tests like this:




dnl
dnl check for libcurl (ESX/XenAPI)
dnl

LIBCURL_CFLAGS=""
LIBCURL_LIBS=""

if test "$with_esx" = "yes" -o "$with_esx" = "check" -o "$with_xenapi"
= "yes" -o "$with_xenapi" = "check"; then
    PKG_CHECK_MODULES(LIBCURL, libcurl >= $LIBCURL_REQUIRED, [
        if test "$with_esx" = "check"; then
            with_esx=yes
        fi

        if test "$with_xenapi" = "check"; then
            with_xenapi=yes
        fi
    ], [
        if test "$with_esx" = "check"; then
            with_esx=no
            AC_MSG_NOTICE([libcurl is required for the ESX driver,
disabling it])
        elif test "$with_esx" = "yes"; then
            AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for
the ESX driver])
        fi

        if test "$with_xenapi" = "check"; then
            with_xenapi=no
            AC_MSG_NOTICE([libcurl is required for the XenAPI driver,
disabling it])
        elif test "$with_xenapi" = "yes"; then
            AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for
the XenAPI driver])
        fi
    ])
fi


if test "$with_esx" = "yes" ; then
    AC_DEFINE_UNQUOTED([WITH_ESX], 1, [whether ESX driver is enabled])
fi
AM_CONDITIONAL([WITH_ESX], [test "$with_esx" = "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"])


AC_SUBST([LIBCURL_CFLAGS])
AC_SUBST([LIBCURL_LIBS])


--this bit has been added and the previous code removed

> @@ -1894,6 +1948,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])
> @@ -1992,6 +2047,12 @@
>  else
>  AC_MSG_NOTICE([     xen: no])
>  fi
> +if test "$with_xenapi" = "yes" ; then
> +AC_MSG_NOTICE([     xenapi:  $LIBXENSERVER_CFLAGS  $LIBXENSERVER_LIBS  $LIBXENSERVER_LDFLAGS])
> +else
> +AC_MSG_NOTICE([     xen: no])

>'xenapi: no' instead of 'xen: no'.
Done

> +fi
>  if test "$with_hal" = "yes" ; then
>  AC_MSG_NOTICE([     hal: $HAL_CFLAGS $HAL_LIBS])
>  else
> --- ./libvirt_org/src/Makefile.am       2010-02-17 17:38:13.000000000 +0000
> +++ ./libvirt/src/Makefile.am   2010-02-26 13:13:59.000000000 +0000
> @@ -205,6 +207,11 @@
>                qemu/qemu_security_dac.h                        \
>                qemu/qemu_security_dac.c
>
> +XENAPI_DRIVER_SOURCES =                                         \
> +                xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
> +                xenapi_driver_private.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 +473,30 @@
>  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   \
> +                     $(LIBXENSERVER_LDFLAGS)

>LIBXENSERVER_LDFLAGS is not necessary here. Adding LIBXENSERVER_LIBS
>to libvirt_driver_xenapi_la_LDFLAGS some lines below should be enough.

LIBXENSERVER_LDFLAGS has been removed from here and the configure script too.

--------------------------PATCH 2/2 BELOW------------------------------------
--- ./libvirt_org/configure.ac  2010-02-17 17:39:21.000000000 +0000
+++ ./libvirt/configure.ac      2010-03-05 13:26:45.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=check@:>@]),[],[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,7 @@
 fi
 AM_CONDITIONAL([WITH_VBOX], [test "$with_vbox" = "yes"])

+
 if test "$with_libvirtd" = "no" ; then
   with_qemu=no
 fi
@@ -335,6 +338,46 @@
 fi
 AM_CONDITIONAL([WITH_LIBVIRTD], [test "$with_libvirtd" = "yes"])

+
+old_LIBS="$LIBS"
+old_CFLAGS="$CFLAGS"
+LIBXENSERVER_LIBS=""
+LIBXENSERVER_CFLAGS=""
+dnl search for the XenServer library
+if test "$with_xenapi" != "no" ; then
+    if test "$with_xenapi" != "yes" -a "$with_xenapi" != "check" ; then
+        LIBXENSERVER_CFLAGS="-I$with_xenapi/include"
+        LIBXENSERVER_LIBS="-L$with_xenapi"
+    fi
+    fail=0
+    CFLAGS="$CFLAGS $LIBXENSERVER_CFLAGS"
+    LIBS="$LIBS $LIBXENSERVER_LIBS"
+    AC_CHECK_LIB([xenserver], [xen_vm_start], [
+           with_xenapi=yes
+           LIBXENSERVER_LIBS="$LIBXENSERVER_LIBS -lxenserver"
+       ],[
+           if test "$with_xenapi" = "yes"; then
+               fail=1
+           fi
+           with_xenapi=no
+       ])
+fi
+
+LIBS="$old_LIBS"
+CFLAGS="$old_CFLAGS"
+
+if test $fail = 1; then
+    AC_MSG_ERROR([You must install the XenServer Library to compile XenAPI driver with -lxenserver])
+fi
+
+if test "$with_xenapi" = "yes"; then
+    AC_DEFINE_UNQUOTED([WITH_XENAPI], 1, [whether XenAPI driver is enabled])
+fi
+
+AC_SUBST([LIBXENSERVER_CFLAGS])
+AC_SUBST([LIBXENSERVER_LIBS])
+
+
 old_LIBS="$LIBS"
 old_CFLAGS="$CFLAGS"
 XEN_LIBS=""
@@ -1440,31 +1483,51 @@
 AC_SUBST([LIBPARTED_LIBS])

 dnl
-dnl check for libcurl (ESX)
+dnl check for libcurl (ESX/XenAPI)
 dnl

 LIBCURL_CFLAGS=""
 LIBCURL_LIBS=""
-LIBCURL_FOUND="no"

-if test "$with_esx" = "yes" -o "$with_esx" = "check"; then
+if test "$with_esx" = "yes" -o "$with_esx" = "check" -o "$with_xenapi"
+= "yes" -o "$with_xenapi" = "check"; then
     PKG_CHECK_MODULES(LIBCURL, libcurl >= $LIBCURL_REQUIRED, [
-        LIBCURL_FOUND=yes
-        with_esx="yes"
+        if test "$with_esx" = "check"; then
+            with_esx=yes
+        fi
+
+        if test "$with_xenapi" = "check"; then
+            with_xenapi=yes
+        fi
     ], [
         if test "$with_esx" = "check"; then
             with_esx=no
-            AC_MSG_NOTICE([libcurl is required for ESX driver, disabling it])
-        else
+            AC_MSG_NOTICE([libcurl is required for the ESX driver, disabling it])
+        elif test "$with_esx" = "yes"; then
             AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for the ESX driver])
         fi
+
+        if test "$with_xenapi" = "check"; then
+            with_xenapi=no
+            AC_MSG_NOTICE([libcurl is required for the XenAPI driver, disabling it])
+        elif test "$with_xenapi" = "yes"; then
+            AC_MSG_ERROR([libcurl >= $LIBCURL_REQUIRED is required for the XenAPI driver])
+        fi
     ])
 fi
+
+
 if test "$with_esx" = "yes" ; then
-    AC_DEFINE_UNQUOTED([WITH_ESX], 1, [whether ESX driver is enabled])
-fi
+    AC_DEFINE_UNQUOTED([WITH_ESX], 1, [whether ESX driver is enabled])
+fi
 AM_CONDITIONAL([WITH_ESX], [test "$with_esx" = "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"])
+
+
 AC_SUBST([LIBCURL_CFLAGS])
 AC_SUBST([LIBCURL_LIBS])

@@ -1894,6 +1957,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])
@@ -1992,6 +2056,11 @@
 else
 AC_MSG_NOTICE([     xen: no])
 fi
+if test "$with_xenapi" = "yes" ; then
+AC_MSG_NOTICE([     xenapi: $LIBXENSERVER_CFLAGS $LIBXENSERVER_LIBS ])
+else
+AC_MSG_NOTICE([     xenapi: no])
+fi
 if test "$with_hal" = "yes" ; then
 AC_MSG_NOTICE([     hal: $HAL_CFLAGS $HAL_LIBS])
 else
--- ./libvirt_org/src/Makefile.am       2010-02-17 17:38:13.000000000 +0000
+++ ./libvirt/src/Makefile.am   2010-03-05 11:49:58.000000000 +0000
@@ -3,6 +3,8 @@
 # No libraries with the exception of LIBXML should be listed
 # here. List them against the individual XXX_la_CFLAGS targets
 # that actually use them
+
+
 INCLUDES =                                                     \
                -I$(top_srcdir)/gnulib/lib                      \
                -I../gnulib/lib                                 \
@@ -205,6 +207,11 @@
                qemu/qemu_security_dac.h                        \
                qemu/qemu_security_dac.c

+XENAPI_DRIVER_SOURCES =                                         \
+                xenapi/xenapi_driver.c xenapi/xenapi_driver.h   \
+                xenapi_driver_private.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 +473,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
+endif
+libvirt_driver_xenapi_la_CFLAGS = $(LIBXENSERVER_CFLAGS)       \
+                                    -I top_srcdir@/src         \
+                                    -I top_srcdir@/src/xenapi  \
+                                    -I top_srcdir@/src/conf    \
+                                  $(LIBCURL_CFLAGS)
+libvirt_driver_xenapi_la_LDFLAGS = $(LIBXENSERVER_LIBS) -g $(LIBXML_LIBS) \
+                                   $(LIBCURL_LIBS)
+
+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 +751,7 @@
                $(OPENVZ_DRIVER_SOURCES)                        \
                $(PHYP_DRIVER_SOURCES)                          \
                $(VBOX_DRIVER_SOURCES)                          \
+               $(XENAPI_DRIVER_SOURCES)                        \
                $(ESX_DRIVER_SOURCES)                           \
                $(NETWORK_DRIVER_SOURCES)                       \
                $(INTERFACE_DRIVER_SOURCES)                     \

--- ./libvirt_org/src/driver.h  2010-02-17 17:38:08.000000000 +0000
+++ ./libvirt/src/driver.h      2010-02-18 10:45:54.000000000 +0000
@@ -27,6 +27,7 @@
     VIR_DRV_ONE = 9,
     VIR_DRV_ESX = 10,
     VIR_DRV_PHYP = 11,
+    VIR_DRV_XENAPI = 12
 } virDrvNo;
--- ./libvirt_org/src/libvirt.c 2010-02-17 17:38:08.000000000 +0000
+++ ./libvirt/src/libvirt.c     2010-02-18 12:21:43.000000000 +0000
@@ -64,6 +64,9 @@
 #ifdef WITH_ESX
 #include "esx/esx_driver.h"
 #endif
+#ifdef WITH_XENAPI
+#include "xenapi/xenapi_driver.h"
+#endif
 #endif

 #define VIR_FROM_THIS VIR_FROM_NONE
@@ -357,6 +360,7 @@
     virDriverLoadModule("openvz");
     virDriverLoadModule("vbox");
     virDriverLoadModule("esx");
+    virDriverLoadModule("xenapi");
     virDriverLoadModule("remote");
 #else
 #ifdef WITH_TEST
@@ -377,6 +381,9 @@
 #ifdef WITH_ESX
     if (esxRegister() == -1) return -1;  #endif
+#ifdef WITH_XENAPI
+    if (xenapiRegister () == -1) return -1; #endif
 #ifdef WITH_REMOTE
     if (remoteRegister () == -1) return -1;  #endif @@ -1035,6 +1042,10 @@
         if (STRCASEEQ(type, "Remote"))
             *typeVer = remoteVersion();  #endif
+#if WITH_XENAPI
+        if (STRCASEEQ(type, "XenAPI"))
+            *typeVer = LIBVIR_VERSION_NUMBER; #endif
         if (*typeVer == 0) {
             virLibConnError(NULL, VIR_ERR_NO_SUPPORT, type);
             goto error;
diff--- ./libvirt_org/include/libvirt/virterror.h   2010-02-17 17:37:51.000000000 +0000
diff+++ ./libvirt/include/libvirt/virterror.h       2010-02-18 12:17:54.000000000 +0000
@@ -69,6 +69,7 @@
     VIR_FROM_PHYP,      /* Error from IBM power hypervisor */
     VIR_FROM_SECRET,    /* Error from secret storage */
     VIR_FROM_CPU,       /* Error from CPU driver */
+    VIR_FROM_XENAPI     /* Error from XenAPI */
 } virErrorDomain;
> diff -ur ./libvirt_org/src/util/virterror.c
> ./libvirt/src/util/virterror.c
> --- ./libvirt_org/src/util/virterror.c  2010-02-17 17:38:14.000000000
> +0000
> +++ ./libvirt/src/util/virterror.c      2010-02-18 12:13:08.000000000 +0000
> @@ -85,6 +85,9 @@
>          case VIR_FROM_XEN:
>              dom = "Xen ";
>              break;
> +        case VIR_FROM_XENAPI:
> +            dom = "XenAPI ";
> +            break;
>          case VIR_FROM_XML:
>              dom = "XML ";
>              break;


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