Re: tools/libxl: fix compilation and link errors on NetBSD

[Christoph Egger <chegger@amazon.de>]

On 14.05.13 18:34, Ian Campbell wrote:
> On Tue, 2013-05-14 at 15:51 +0100, Christoph Egger wrote:
>> commit cad172d7b88bd443c81d865051297875ce2551bc
>> Author: Christoph Egger <chegger@amazon.de>
>> 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@amazon.de>
>>     Reviewed-by: Matthew Wilson <msw@amazon.de>
>>
>> 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.

I see the following link failure:

gcc    -pthread -o testidl testidl.o libxlutil.so /home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so -Wl,-rpath-link=/home/chegger/xen.git/tools/libxl/../../tools/libxc -Wl,-rpath-link=/home/chegger/xen.git/tools/libxl/../../tools/xenstore  /home/chegger/xen.git/tools/libxl/../../tools/libxc/libxenctrl.so  -L/usr/pkg/lib
ld: warning: libyajl.so.2, needed by /home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so, not found (try using -rpath or -rpath-link)
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_parse'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_complete_parse'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_null'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_array_open'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_number'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_string'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_map_close'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_get_buf'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_double'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_config'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_free'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_alloc'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_array_close'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_map_open'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_get_error'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_free_error'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_integer'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_alloc'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_free'
/home/chegger/xen.git/tools/libxl/../../tools/libxl/libxenlight.so: undefined reference to `yajl_gen_bool'
gmake[3]: *** [testidl] Error 1
gmake[3]: Leaving directory `/home/chegger/xen.git/tools/libxl'
gmake[2]: *** [subdir-install-libxl] Error 2
gmake[2]: Leaving directory `/home/chegger/xen.git/tools'
gmake[1]: *** [subdirs-install] Error 2
gmake[1]: Leaving directory `/home/chegger/xen.git/tools'
gmake: *** [install-tools] Error 2


> 
> 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.

This change is just that -lyajl is not redundant here.

Adding -lyajl to testidl like to xl w/o my patch doesn't work for me. I see the
same error as above.

> 
>>  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.

Bah. I need to find a better way to transfer patches from my development machine through
the internal review process to my mail client.
On my development machine I have the missing 's'. I just checked.

Christoph