Re: [Xen-devel] tools/libxl: fix compilation and link errors on NetBSD

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-05-14 at 15:51 +0100, Christoph Egger wrote:
> commit cad172d7b88bd443c81d865051297875ce2551bc
> Author: Christoph Egger <chegger@xxxxxxxxx>
> Date:   Thu Feb 7 14:42:29 2013 +0000
> 
>     tools/libxl: fix compilation and link errors on NetBSD
>     
>     - Fix testidl link error that libyajl is not found
>     - Make linking of xl and testidl consistent
>     - fix error: array subscript has type 'char'
>     
>     Signed-off-by: Christoph Egger <chegger@xxxxxxxxx>
>     Reviewed-by: Matthew Wilson <msw@xxxxxxxxx>
> 
> diff --git a/tools/Rules.mk b/tools/Rules.mk
> index 3f03a31..4067955 100644
> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -56,7 +56,7 @@ SHLIB_libblktapctl  =
>  endif
>  
>  CFLAGS_libxenlight = -I$(XEN_XENLIGHT) $(CFLAGS_libxenctrl) 
> $(CFLAGS_xeninclude)
> -LDLIBS_libxenlight = $(XEN_XENLIGHT)/libxenlight.so $(SHLIB_libxenctrl) 
> $(SHLIB_libxenstore) $(SHLIB_libblktapctl)
> +LDLIBS_libxenlight = $(XEN_XENLIGHT)/libxenlight.so $(APPEND_LDFLAGS) -lyajl 
> $(SHLIB_libxenctrl) $(SHLIB_libxenstore) $(SHLIB_libblktapctl)

I don't know what compile/link failure you are seeing on NetBSD but I'm
afraid I don't agree this is a correct fix.

This variable specifies the linker line which an application linking
libxl is required to use. Unless that application is using yajl itself
then it doesn't need -lyajl since the library is correctly linked to the
libyajl itself (via ELF DT_NEEDED).

If an application does use yajl itself (as xl does) then it should also
link against the library itself, which is what xl does (or did before
you changed it below), but it should not get this from
LDLIBS_libxenlight.

testidl.c doesn't use yajl directly, so it shouldn't need to link
against libjyajl itself directly AFAICT. The fact that testidl and xl
use different libraries is exactly the reason why their link lines are
"inconsistent".

With GNU ld this relates the the --as-needed option, perhaps you need to
find similar functionality for your linker?

Also APPEND_LDFLAGS should be in the final app link only, no need to add
it here.

>  SHLIB_libxenlight  = -Wl,-rpath-link=$(XEN_XENLIGHT)
>  
>  CFLAGS += -D__XEN_TOOLS__
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index cf214bb..c65c11e 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -189,7 +189,7 @@ libxlutil.a: $(LIBXLU_OBJS)
>         $(AR) rcs libxlutil.a $^
>  
>  xl: $(XL_OBJS) libxlutil.so libxenlight.so
> -       $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) libxlutil.so $(LDLIBS_libxenlight) 
> $(LDLIBS_libxenctrl) -lyajl $(APPEND_LDFLAGS)
> +       $(CC) $(LDFLAGS) -o $@ $(XL_OBJS) libxlutil.so $(LDLIBS_libxenlight) 
> $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)

As explained above you shouldn't be changing this. I suppose it might be
possible that reordering the yajl here is a correct fix for your issue,
but having not seen it I can't really say.

>  libxl-save-helper: $(SAVE_HELPER_OBJS) libxenlight.so
>         $(CC) $(LDFLAGS) -o $@ $(SAVE_HELPER_OBJS) $(LDLIBS_libxenctrl) 
> $(LDLIBS_libxenguest) $(APPEND_LDFLAGS)
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 35da71c..0c039b1 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -95,7 +95,7 @@ int libxl_domain_qualifier_to_domid(libxl_ctx *ctx, const 
> char *name,
>  {
>      int i, rv;
>      for (i=0; name[i]; i++) {
> -        if (!isdigit(name[i])) {
> +        if (!isdigit((unigned char)name[i])) {

You very obviously never even compile tested this.

Ian.


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