Re: [Qemu-devel] [PATCH V11 6/7] libqblock API implement

[Wenchao Xia]

> Il 24/11/2012 10:27, Wenchao Xia ha scritto:
> >    This patch contains implemention for APIs.
> > Important APIs:
> >    1 QBlockContext. This structure was used to retrieve errors, every thread
> > must create one first.
> >    2 QBlockImage. It stands for an block image object.
> >    3 QBlockStaticInfo. It contains static information such as location, backing
> > file, size.
> >    4 Sync I/O. It is similar to C file open, read, write and close operations.
> >
> > v11:
> >    Moved API design out of this patch.
> >    Spell fix.
> >    Use a new function in libqblock-aio.c to do bdrv init and aio init, removed
> > this section from library loading call back function, which allows map different
> > aio-context to differenct QBlockContext in the future.
> >    Renamed QBlockState to QBlockImage.
> >    Added reference counter in QBlockImage, which is only used in new/delete pair
> > function now.
>
> With a reference count, the API should be new/ref/unref, not new/delete.
>   The "delete" function should be internal, just call it from unref when
> the reference count goes to zero.
>
> Some other comments below.
  OK, missed your point before, will change the API to new/ref/unref.


> >    Removed useless parentheses around & argument.
> >    Move virt_size out of format unions, removed virt_size from 
> > QBlockStaticInfoAddr.
> >    bdrv_read and bdrv_write, Report I/O error when block api return negative 
> > value.
> >    qb_check_allocation, fixed the length check condition and added comments for
> > it.
> >    qb_info_image_static_get, renamed info to p_info and info_tmp to info, also
> > renamed all double pointing parameters with prefix “p_”in all API.
> >    qb_str2fmttype and qb_fmttype2str, added parameter check.
> >    qb_setup_info_addr, moved memset into it.
> >    qb_info_image_static_get, added format valid check, removed variable 
> > member_addr,
> > moved memset to qb_setup_info_addr.
> >
> > Signed-off-by: Wenchao Xia <address@hidden>
> > ---
> >   libqblock/libqblock-aio.c   |  110 ++++-
> >   libqblock/libqblock-error.c |   57 ++
> >   libqblock/libqblock.c       | 1191 ++++++++++++++++++++++++++++++++++++++++++-
> >   3 files changed, 1349 insertions(+), 9 deletions(-)
> >
> > diff --git a/libqblock/libqblock-aio.c b/libqblock/libqblock-aio.c
> > index 605eee8..97d7ad9 100644
> > --- a/libqblock/libqblock-aio.c
> > +++ b/libqblock/libqblock-aio.c
> > @@ -11,31 +11,63 @@
> >    *
> >    */
> >
> > +/* This file was only used in libqblock, codes are copied from main-loop.c,
> > + iohandler.c, compatfd.c now, it may have different implemention in the future.
> > +*/
> > +
> >   #include <sys/syscall.h>
> >
> > +#include "libqblock-aio.h"
> > +
> >   #include "qemu-common.h"
> >   #include "qemu-aio.h"
> >   #include "main-loop.h"
> >   #include "compatfd.h"
> >
> > -void qemu_notify_event(void)
> > +#include "block.h"
> > +
> > +/* Aio support, copied from main-loop.c */
> > +
> > +static AioContext *qemu_aio_context;
> > +
> > +/* This function should only be called once now. */
> > +void libqblock_aio_init(void)
> >   {
> > +    GSource *src;
> > +
> > +    qemu_aio_context = aio_context_new();
> > +    /* bdrv_init must be called after qemu_aio_context was set. */
> > +    bdrv_init();
> > +
> > +    src = aio_get_g_source(qemu_aio_context);
> > +    g_source_attach(src, NULL);
> > +    g_source_unref(src);
>
> There is no need (yet) to do these three steps.  Once we add an
> asynchronous I/O API, we can add an API to get an AioContext from a
> QBlockContext.
>
> This however requires support for multiple AioContexts, I think.  So
> that's left for later.
  It seems aio_context_new() and bdrv_init() must be called to make sure
block API works, should this function be deleted now and move this
initial calls into another new function libqblock_init()?

> >       return;
> >   }


> > +
> > +/* Signal fd support, copied from compatfd.c */
> > +
> >   bool qemu_signalfd_available(void)
> >   {
> > +#ifdef CONFIG_SIGNALFD
> > +    sigset_t mask;
> > +    int fd;
> > +    bool ok;
> > +    sigemptyset(&mask);
> > +    errno = 0;
> > +    fd = syscall(SYS_signalfd, -1, &mask, _NSIG / 8);
> > +    ok = (errno != ENOSYS);
> > +    if (fd >= 0) {
> > +        close(fd);
> > +    }
> > +    return ok;
> > +#else
> >       return false;
> > +#endif
> >   }
>
> Please just return false.
  OK.


> > @@ -77,5 +171,5 @@ int qemu_set_fd_handler(int fd,
> >                           IOHandler *fd_write,
> >                           void *opaque)
> >   {
> > -    return 0;
> > +    return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> >   }
>
> The stubs/ version is enough for qemu_set_fd_handler2. The only use of
> qemu_set_fd_handler is in event_notifier-posix.c, either change it to
> qemu_set_fd_handler2 or add another file to stubs/.
  OK.



--
Best Regards

Wenchao Xia