Re: [Qemu-devel] [PATCH V10 4/7] libqblock build system

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 2012-11-20 18:26, Paolo Bonzini 写道:
> Mostly looks good...
>
> Il 20/11/2012 10:46, Wenchao Xia ha scritto:
>> +libqblock="yes"
>
> Please make the default the empty string, i.e. "build if and only if
> libtool is present".
>
   Do you mean adding a probe section later that if libtool is present,
its default is "yes", else set it to "no"?


>>
>> +##########################################
>> +# libqblock probe
>> +if test "$libqblock" != "no" ; then
>> +# libqblock depends on libtool, default to yes if libtool exist
>> +    if ! has $libtool; then
>> +        echo
>> +        echo "Warning: libqblock needs libtool"
>> +        echo "Setting libqblock option to no."
>> +        echo
>> +        libqblock="no"
>> +    fi
>> +fi
>>   #
>
> Please fail here if libqblock=yes but libtool is not found.
>
   OK.

>> +#library objects
>> +libqblock-obj-y= libqblock/libqblock.o libqblock/libqblock-error.o
>> +extra-obj-y= main-loop.o qemu-timer.o qemu-tool.o iohandler.o \
>> +             compatfd.o
>
> Are all of these really necessary?  Instead of
> main-loop/compatfd/iohandler, I think you can just add a new
> libqblock/libqblock-aio.c file, and copy these definitions from main-loop.c:
>
   OK, good idea.

> QEMUBH *qemu_bh_new(QEMUBHFunc *cb, void *opaque)
> void qemu_aio_flush(void)
> bool qemu_aio_wait(void)
> void qemu_aio_set_fd_handler(int fd,
>                              IOHandler *io_read,
>                              IOHandler *io_write,
>                              AioFlushHandler *io_flush,
>                              void *opaque);
> void qemu_aio_set_event_notifier(EventNotifier *notifier,
>                                  EventNotifierHandler *io_read,
>                                  AioFlushEventNotifierHandler *io_flush);
>
> and also include part of qemu_init_main_loop in the creation of a new
> QBlockContext (the first time it's called).  We can later map each
> QBlockContext to a different AioContext.  Please document somewhere that
> the library is not yet thread-safe.
>
   OK.

>> diff --git a/tests/Makefile b/tests/Makefile
>> index ef6c9f2..997dbcc 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -84,6 +84,23 @@ check-qtest-$(CONFIG_POSIX)=$(foreach TARGET,$(TARGETS), $(check-qtest-$(TARGET)
>>   qtest-obj-y = tests/libqtest.o $(oslib-obj-y) libqemustub.a
>>   $(check-qtest-y): $(qtest-obj-y)
>>
>> +#libqblock build rules
>> +check-libqblock-$(CONFIG_LIBQBLOCK) = tests/check-libqblock-qcow2$(EXESUF)
>> +
>> +$(check-libqblock-y): QEMU_INCLUDES += -I$(SRC_PATH)/tests -I$(SRC_PATH)/$(LIBQBLOCK_DIR)
>> +LIBQBLOCK_TEST_DIR = tests/test_images
>> +LIBQBLOCK_DIR = libqblock
>> +LIBQBLOCK_SO = $(LIBQBLOCK_DIR)/.libs/libqblock.so
>> +LIBQBLOCK_SO_LINK_FLAG = -Wl,-rpath,$(LIBQBLOCK_DIR)/.libs
>
> These two should not be required.  Just link in libqblock/libqblock.la
> and libtool will do all of its magic.
>
   OK.

> Paolo
>
>> +
>> +#use libtool to link check-libqblock, use LIBQBLOCK_SO_LINK_FLAG to add lib search diretory
>> +LTLINK_CHECK_LIBQBLOCK = $(call quiet-command, $(LIBTOOL) --mode=link --quiet --tag=CC \
>> +         $(CC) $(QEMU_CFLAGS) $(CFLAGS) $(LDFLAGS) $(LIBQBLOCK_SO_LINK_FLAG) \
>> +         -o $@ $(sort $(1)) $(LIBS), "lt LINK $(TARGET_DIR) $@")
>> +
>> +$(check-libqblock-y): %$(EXESUF): %.o
>> +	$(call LTLINK_CHECK_LIBQBLOCK, $^ $(LIBQBLOCK_SO))
>
>


-- 
Best Regards

Wenchao Xia