Re: [Qemu-devel] [PATCH 01/20] tests: fix linking test-char on win32

[Eric Blake]

On 01/06/2017 01:50 PM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 01/05/2017 10:53 AM, Marc-André Lureau wrote:
>>> test-char.exe needs main-loop.o symbols, among others. Linking with
>>> $(test-block-obj-y) brings what's necessary. We could try to eventually
>>> strip to the minimum if needed.
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> ---
>>>  tests/Makefile.include | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Can I have a bit more background? I'm guessing this doesn't fix an
>> actual failure at the moment (probably due to one of the other files
>> dragging in stuff indirectly), but rather cleans up a latent problem
>> that would otherwise be exposed with patches later in the series (where
>> splitting files means the stuff is no longer dragged in indirectly, so
>> we need to mention it explicitly)?
> 
> It does fix current linking failure:
> x86_64-w64-mingw32-g++ 
> -I/usr/x86_64-w64-mingw32/sys-root/mingw/include/pixman-1  
> -I/home/elmarco/src/qemu/dtc/libfdt -Werror -mms-bitfields 
> -I/usr/x86_64-w64-mingw32/sys-root/mingw/include/glib-2.0 
> -I/usr/x86_64-w64-mingw32/sys-root/mingw/lib/glib-2.0/include 
> -I/usr/x86_64-w64-mingw32/sys-root/mingw/include  -m64 -mcx16 -mthreads 
> -D__USE_MINGW_ANSI_STDIO=1 -DWIN32_LEAN_AND_MEAN -DWINVER=0x501 -D_GNU_SOURCE 
> -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
> -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
> -fno-strict-aliasing -fno-common -fwrapv  -Wendif-labels 
> -Wno-shift-negative-value -Wmissing-include-dirs -Wempty-body 
> -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self 
> -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition 
> -Wtype-limits -fstack-protector-strong  
> -I/usr/x86_64-w64-mingw32/sys-root/mingw/include/libpng16  
> -I/home/elmarco/src/qemu/tests -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g  
> -Wl,--nxcompat -Wl,--no-seh -Wl,--dynamicbase -Wl,--warn-common -m64 -g  -o 
> tests/test-char.exe tests/test-char.o qemu-char.o qemu-timer.o io/channel.o 
> io/channel-buffer.o io/channel-command.o io/channel-file.o 
> io/channel-socket.o io/channel-tls.o io/channel-watch.o io/channel-websock.o 
> io/channel-util.o io/task.o crypto/init.o crypto/hash.o crypto/hash-glib.o 
> crypto/hmac.o crypto/hmac-glib.o crypto/aes.o crypto/desrfb.o crypto/cipher.o 
> crypto/tlscreds.o crypto/tlscredsanon.o crypto/tlscredsx509.o 
> crypto/tlssession.o crypto/secret.o crypto/random-platform.o crypto/pbkdf.o 
> crypto/ivgen.o crypto/ivgen-essiv.o crypto/ivgen-plain.o 
> crypto/ivgen-plain64.o crypto/afsplit.o crypto/xts.o crypto/block.o 
> crypto/block-qcow.o crypto/block-luks.o qom/object.o qom/container.o 
> qom/qom-qobject.o qom/object_interfaces.o  libqemuutil.a libqemustub.a 
> /home/elmarco/src/qemu/winb/version.o  
> -L/usr/x86_64-w64-mingw32/sys-root/mingw/lib -lgthread-2.0 -lglib-2.0 -lintl  
> -lwinmm -lws2_32 -liphlpapi  -lz -lz 
> qemu-char.o: In function `win_chr_free':
> /home/elmarco/src/qemu/qemu-char.c:2149: undefined reference to 
> `qemu_del_polling_cb'

Ah, so it's an existing problem in mingw compilation (I missed that
part; I assumed that because it links fine on Linux that there was no
problem - without realizing that the mingw implementation of char
devices drags in more requirements thanks to the #ifdef's char drivers).

> I can add this linking error to the commit log if it helps.

Yes, listing the link failure in the commit message (either just those
two lines above, or the whole she-bang) helps tremendously.

>>
>> Even mentioning _which_ main-loop.o symbol to be grepping for in
>> test-char.c might be helpful to the review.
> 
> It's from qemu-char.c.

Yep, all clear now.

>> At any rate, test-block-obj-y is simply test-io-obj-y plus the
>> additional block-obj-y, so you're merely adding an additional dependency
>> on block-obj-y files.  While a nicer commit message may help, I don't
>> see any technical problem in the change, so here's a weak:
>>
>> Reviewed-by: Eric Blake <address@hidden>

And with the commit message improvement, it is now a strong R-b.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature