Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed

[Cao jin <caoj.fnst@cn.fujitsu.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 <caoj.fnst@cn.fujitsu.com>
>>> ---
>>>   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 <sys/epoll.h>
>>>   #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