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

[Paolo Bonzini <pbonzini@redhat.com>]

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

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

> +#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:

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.

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

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