From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42848) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bN9Ix-0006SG-C6 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 21:49:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bN9Iu-0002IT-AG for qemu-devel@nongnu.org; Tue, 12 Jul 2016 21:49:39 -0400 References: <1468323255-20454-1-git-send-email-caoj.fnst@cn.fujitsu.com> <57850FB6.3000403@redhat.com> <20160713013352.GA16038@ad.usersys.redhat.com> From: Cao jin Message-ID: <57859FA8.5060106@cn.fujitsu.com> Date: Wed, 13 Jul 2016 09:55:52 +0800 MIME-Version: 1.0 In-Reply-To: <20160713013352.GA16038@ad.usersys.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , Eric Blake Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, stefanha@redhat.com On 07/13/2016 09:33 AM, Fam Zheng wrote: > On Tue, 07/12 09:41, Eric Blake wrote: >> On 07/12/2016 05:34 AM, Cao jin wrote: >>> The current judegement of caller is meaningless, make it useful. >> >> Is this something you can trigger? If so, what command line? If not, >> how did you find it? >> >> Spelled 'judgment', not 'judegement'; but rather than just fixing the >> typo, it might read better as: >> >> The lone caller of aio_context_setup() only checks whether an error was >> set, but aio_context_setup() is not setting an error. >> >>> >>> Signed-off-by: Cao jin >>> --- >>> aio-posix.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/aio-posix.c b/aio-posix.c >>> index 6006122..8b0deb7 100644 >>> --- a/aio-posix.c >>> +++ b/aio-posix.c >>> @@ -18,6 +18,7 @@ >>> #include "block/block.h" >>> #include "qemu/queue.h" >>> #include "qemu/sockets.h" >>> +#include "qapi/error.h" >>> #ifdef CONFIG_EPOLL_CREATE1 >>> #include >>> #endif >>> @@ -491,6 +492,7 @@ void aio_context_setup(AioContext *ctx, Error **errp) >>> assert(!ctx->epollfd); >>> ctx->epollfd = epoll_create1(EPOLL_CLOEXEC); >>> if (ctx->epollfd == -1) { >>> + error_setg_errno(errp, errno, "Failed to create epoll instance"); >>> ctx->epoll_available = false; >>> } else { >>> ctx->epoll_available = true; >> >> However, I'm not even sure your patch is right. The mere fact that we >> have ctx->epoll_available makes it sound like we WANT to be able to >> gracefully fall back when epoll is not available, rather than raising an >> error message. > > Yes. This patch doesn't make sense to me. What can be done is 1) remove errp > parameter as it's not used; 2) fprintf(stderr, ...) or error_report(....) here > if epoll_create1 failed. > > Fam > Actually, my first thought is 1), and I also checked how ctx->epoll_available is used(seems I still didn't read it carefully). But when I see how event_notifier_init()'s failure(in aio_context_new()) is handled, I think maybe keeping consistence is better. ok, I will choose 1) -- Yours Sincerely, Cao jin