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

[Wenchao Xia]

于 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