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

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

 > 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 <xiawenc@linux.vnet.ibm.com>
>> ---
>>   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