From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdBmK-0003ZQ-CB for qemu-devel@nongnu.org; Mon, 26 Nov 2012 22:24:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TdBmJ-0007B2-1z for qemu-devel@nongnu.org; Mon, 26 Nov 2012 22:24:08 -0500 Received: from e23smtp07.au.ibm.com ([202.81.31.140]:42738) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TdBmI-0007AW-Fi for qemu-devel@nongnu.org; Mon, 26 Nov 2012 22:24:06 -0500 Received: from /spool/local by e23smtp07.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 27 Nov 2012 13:19:36 +1000 Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qAR3O0AO66060500 for ; Tue, 27 Nov 2012 14:24:00 +1100 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qAR3Nxac001180 for ; Tue, 27 Nov 2012 14:24:00 +1100 Message-ID: <50B431FD.8050809@linux.vnet.ibm.com> Date: Tue, 27 Nov 2012 11:22:37 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1353749244-25676-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1353749244-25676-7-git-send-email-xiawenc@linux.vnet.ibm.com> <50B32979.6010905@redhat.com> In-Reply-To: <50B32979.6010905@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V11 6/7] libqblock API implement List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, stefanha@gmail.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.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 >> --- >> 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 >> >> +#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