* [Qemu-devel] [PATCH] aio-posix: fill error message that is missed @ 2016-07-12 11:34 Cao jin 2016-07-12 15:41 ` Eric Blake 0 siblings, 1 reply; 5+ messages in thread From: Cao jin @ 2016-07-12 11:34 UTC (permalink / raw) To: qemu-devel; +Cc: stefanha, famz, qemu-block The current judegement of caller is meaningless, make it useful. 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; -- 2.1.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed 2016-07-12 11:34 [Qemu-devel] [PATCH] aio-posix: fill error message that is missed Cao jin @ 2016-07-12 15:41 ` Eric Blake 2016-07-13 1:33 ` Fam Zheng 2016-07-13 1:42 ` Cao jin 0 siblings, 2 replies; 5+ messages in thread From: Eric Blake @ 2016-07-12 15:41 UTC (permalink / raw) To: Cao jin, qemu-devel; +Cc: famz, qemu-block, stefanha [-- Attachment #1: Type: text/plain, Size: 1607 bytes --] 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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed 2016-07-12 15:41 ` Eric Blake @ 2016-07-13 1:33 ` Fam Zheng 2016-07-13 1:55 ` Cao jin 2016-07-13 1:42 ` Cao jin 1 sibling, 1 reply; 5+ messages in thread From: Fam Zheng @ 2016-07-13 1:33 UTC (permalink / raw) To: Eric Blake; +Cc: Cao jin, qemu-devel, qemu-block, stefanha 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed 2016-07-13 1:33 ` Fam Zheng @ 2016-07-13 1:55 ` Cao jin 0 siblings, 0 replies; 5+ messages in thread From: Cao jin @ 2016-07-13 1:55 UTC (permalink / raw) To: Fam Zheng, Eric Blake; +Cc: qemu-devel, qemu-block, stefanha 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] aio-posix: fill error message that is missed 2016-07-12 15:41 ` Eric Blake 2016-07-13 1:33 ` Fam Zheng @ 2016-07-13 1:42 ` Cao jin 1 sibling, 0 replies; 5+ messages in thread From: Cao jin @ 2016-07-13 1:42 UTC (permalink / raw) To: Eric Blake, qemu-devel; +Cc: famz, qemu-block, stefanha On 07/12/2016 11:41 PM, 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? > No, I didn't find it by "trigger" it. I just a very carefully code reader when I learn 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. > Ok >> >> 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. > -- Yours Sincerely, Cao jin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-13 1:49 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-12 11:34 [Qemu-devel] [PATCH] aio-posix: fill error message that is missed Cao jin 2016-07-12 15:41 ` Eric Blake 2016-07-13 1:33 ` Fam Zheng 2016-07-13 1:55 ` Cao jin 2016-07-13 1:42 ` Cao jin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).