Re: [Xen-devel] [xen-devel] [PATCH] libxl: fix compile error of libvirt

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(I could have sworn I hit send on this. apologies if you get it twice
somehow)

On Fri, 2012-02-24 at 15:42 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [xen-devel] [PATCH] libxl: fix compile 
> error of libvirt"):
> > We should arrange for xl*.c to not have __XEN_TOOLS__ defined though.
> > 
> > -D__XEN_TOOLS__ is part of the base CFLAGS defined in tools/Rules.mk but
> > perhaps we could add -U__XEN_TOOLS__ to the xl specific cflags?
> 
> That would be a possibility but if we define it in libxl.h that will
> fix the problem, if we are happy for libxl callers to be using
> hypervisor public headers.
> 
> > We can stop xl by just not doing it (TM), for other callers of libxl --
> > well, we say you shouldn't need it for toolstack operations and that
> > libxl should provide all the functionality but if they _really_ want to
> > ignore that...
> > 
> > Also a toolstack may have functionality which is not considered
> > "toolstack functionality" per the remit of libxl and for which they wish
> > to use xenctrl directly. e.g. perhaps they communicate with a guest
> > agent using their own shared ring protocol for which they require the
> > ability to map domain memory.
> 
> How about the patch below.  This would be an alternative to your
> version which buries all the hypervisor public headers.

It's clever, some might say too clever ;-)

I'm a bit concerned about the use of domctl and sysctl in the libxl API
anyway -- I'm not convinced those are guaranteed to be stable ABIs by
the hypervisor (in fact, I'm reasonably sure they are not -- hence the
#error), in which case we mustn't expose them to libxl's users.

sched.h concerns me less -- that one is guest visible API IIRC and
therefore stable. But sched.h doesn't have a #error in it so isn't a
problem.

So I think we have to take the first of my patches. At which point we
can avoid the twisty maze of your patch using -U... as necessary.

Ian.

> 
> Ian.
> 
> From: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Subject: [PATCH] libxl: Forbid <xenctrl.h>, allow Xen public headers
> 
> Rationalise and enforce the header-inclusion policy for libxl.
> 
> libxl applications are allowed to use symbols (mostly #defines) from
> the Xen public headers, where these are useful (for example, where
> they are the numeric arguments to libxl calls).  This avoids us having
> to veneer the whole of the Xen public API.  For this to work without
> special measures on the part of the application, libxl.h should define
> __XEN_TOOLS__.
> 
> However libxl applications are not normally expected to want to use
> libxc directly, so take steps to prevent them including <xenctrl.h>
> unless they declare (as libxl itself does) that doing so is OK by
> defining LIBXL_ALLOW_XENCTRL.  (We have to add a hook at the end of
> xenctrl.h so that we can spot the case where xenctrl.h is included
> second.)
> 
> Make xc.c comply with the policy: remove the redundant inclusion of
> xenctrl.h.
> 
> This patch should make life easier for out-of-tree libxl callers and
> hopefully prevent future mistakes relating to api visibility.
> 
> Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> 
> ---
>  tools/libxc/xenctrl.h        |    4 ++++
>  tools/libxl/libxl.h          |   10 ++++++++++
>  tools/libxl/libxl_internal.h |    2 ++
>  tools/libxl/xl.c             |    1 -
>  4 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 73d24e5..8441b62 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -2047,4 +2047,8 @@ int xc_compression_uncompress_page(xc_interface *xch, 
> char *compbuf,
>                                  unsigned long compbuf_size,
>                                  unsigned long *compbuf_pos, char *dest);
>  
> +#ifdef XENCTRL_DO_INCLUDE
> +#include XENCTRL_DO_INCLUDE
> +#endif
> +
>  #endif /* XENCTRL_H */
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 1bffa03..86a308d 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -124,9 +124,19 @@
>   * Therefore public functions which initialize a libxl__gc MUST call
>   * libxl__free_all() before returning.
>   */
> +
> +#if !defined(LIBXL_ALLOW_XENCTRL)
> +#ifdef XENCTRL_H
> +#error applications which use libxl should not normally use xenctrl.h too
> +#endif
> +#define XENCTRL_DO_INCLUDE <libxl.h> /* spot if xenctrl.h came second */
> +#endif /* !defined(LIBXL_ALLOW_XENCTRL) */
> +
>  #ifndef LIBXL_H
>  #define LIBXL_H
>  
> +#define __XEN_TOOLS__ 1
> +
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdarg.h>
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 46e131a..478de48 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -17,6 +17,8 @@
>  #ifndef LIBXL_INTERNAL_H
>  #define LIBXL_INTERNAL_H
>  
> +#define LIBXL_ALLOW_XENCTRL
> +
>  #include "libxl_osdeps.h" /* must come before any other headers */
>  
>  #include <assert.h>
> diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
> index df9b1e7..2b14814 100644
> --- a/tools/libxl/xl.c
> +++ b/tools/libxl/xl.c
> @@ -26,7 +26,6 @@
>  #include <fcntl.h>
>  #include <ctype.h>
>  #include <inttypes.h>
> -#include <xenctrl.h>
>  
>  #include "libxl.h"
>  #include "libxl_utils.h"



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel