| Message ID | 1526523977-73930-1-git-send-email-wangjie88@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/2] iothread: fix epollfd leak in the process of delIOThread | expand |
On Thu, 05/17 10:26, Jie Wang wrote: > epoll_available will only be set if epollfd != -1, os we s/os/so/ > can swap the two variables in aio_epoll_disable, and > aio_context_destroy can call aio_epoll_disable directly. If you put this as 1/2 in v7, you do not want to mention the yet-to-be-introduced aio_context_destroy this way. Maybe you can write as "the coming aio_context_destroy can call it directly.". Fam > > Signed-off-by: Jie Wang <wangjie88@huawei.com> > --- > util/aio-posix.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/util/aio-posix.c b/util/aio-posix.c > index 0ade2c7..118bf57 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -45,11 +45,11 @@ struct AioHandler > > static void aio_epoll_disable(AioContext *ctx) > { > - ctx->epoll_available = false; > - if (!ctx->epoll_enabled) { > + ctx->epoll_enabled = false; > + if (!ctx->epoll_available) { > return; > } > - ctx->epoll_enabled = false; > + ctx->epoll_available = false; > close(ctx->epollfd); > } > > @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) > void aio_context_destroy(AioContext *ctx) > { > #ifdef CONFIG_EPOLL_CREATE1 > - if (ctx->epollfd >= 0) { > - close(ctx->epollfd); > - } > + aio_epoll_disable(ctx); > #endif > } > > -- > 1.8.3.1 >
On Thu, May 17, 2018 at 10:26:17AM +0800, Jie Wang wrote: > epoll_available will only be set if epollfd != -1, os we > can swap the two variables in aio_epoll_disable, and > aio_context_destroy can call aio_epoll_disable directly. > > Signed-off-by: Jie Wang <wangjie88@huawei.com> > --- > util/aio-posix.c | 10 ++++------ > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/util/aio-posix.c b/util/aio-posix.c > index 0ade2c7..118bf57 100644 > --- a/util/aio-posix.c > +++ b/util/aio-posix.c > @@ -45,11 +45,11 @@ struct AioHandler > > static void aio_epoll_disable(AioContext *ctx) > { > - ctx->epoll_available = false; > - if (!ctx->epoll_enabled) { > + ctx->epoll_enabled = false; > + if (!ctx->epoll_available) { > return; > } > - ctx->epoll_enabled = false; > + ctx->epoll_available = false; > close(ctx->epollfd); > } > > @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) > void aio_context_destroy(AioContext *ctx) > { > #ifdef CONFIG_EPOLL_CREATE1 > - if (ctx->epollfd >= 0) { > - close(ctx->epollfd); > - } > + aio_epoll_disable(ctx); Hmm... I think this patch should be the first if to split. Anyway, IMHO version 5 is already good enough and has got r-bs, no need to bother reposting a version 7. Maintainer could possibly touch up the commit message if necessary. Thanks, > #endif > } > > -- > 1.8.3.1 >
I enjoyed the great benefit of your suggestions, and I will improve next time. :) This time, I ask maintainers to touch up the commit message base on version 5 and merge it, thanks very much. On 2018/5/17 14:22, Peter Xu wrote: > On Thu, May 17, 2018 at 10:26:17AM +0800, Jie Wang wrote: >> epoll_available will only be set if epollfd != -1, os we >> can swap the two variables in aio_epoll_disable, and >> aio_context_destroy can call aio_epoll_disable directly. >> >> Signed-off-by: Jie Wang <wangjie88@huawei.com> >> --- >> util/aio-posix.c | 10 ++++------ >> 1 file changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/util/aio-posix.c b/util/aio-posix.c >> index 0ade2c7..118bf57 100644 >> --- a/util/aio-posix.c >> +++ b/util/aio-posix.c >> @@ -45,11 +45,11 @@ struct AioHandler >> >> static void aio_epoll_disable(AioContext *ctx) >> { >> - ctx->epoll_available = false; >> - if (!ctx->epoll_enabled) { >> + ctx->epoll_enabled = false; >> + if (!ctx->epoll_available) { >> return; >> } >> - ctx->epoll_enabled = false; >> + ctx->epoll_available = false; >> close(ctx->epollfd); >> } >> >> @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) >> void aio_context_destroy(AioContext *ctx) >> { >> #ifdef CONFIG_EPOLL_CREATE1 >> - if (ctx->epollfd >= 0) { >> - close(ctx->epollfd); >> - } >> + aio_epoll_disable(ctx); > > Hmm... I think this patch should be the first if to split. > > Anyway, IMHO version 5 is already good enough and has got r-bs, no > need to bother reposting a version 7. Maintainer could possibly touch > up the commit message if necessary. > > Thanks, > >> #endif >> } >> >> -- >> 1.8.3.1 >> >
Ping On 2018/5/17 14:48, WangJie (Pluto) wrote: > I enjoyed the great benefit of your suggestions, and I will improve next time. :) > This time, I ask maintainers to touch up the commit message base on version 5 and merge it, thanks very much. > > On 2018/5/17 14:22, Peter Xu wrote: >> On Thu, May 17, 2018 at 10:26:17AM +0800, Jie Wang wrote: >>> epoll_available will only be set if epollfd != -1, os we >>> can swap the two variables in aio_epoll_disable, and >>> aio_context_destroy can call aio_epoll_disable directly. >>> >>> Signed-off-by: Jie Wang <wangjie88@huawei.com> >>> --- >>> util/aio-posix.c | 10 ++++------ >>> 1 file changed, 4 insertions(+), 6 deletions(-) >>> >>> diff --git a/util/aio-posix.c b/util/aio-posix.c >>> index 0ade2c7..118bf57 100644 >>> --- a/util/aio-posix.c >>> +++ b/util/aio-posix.c >>> @@ -45,11 +45,11 @@ struct AioHandler >>> >>> static void aio_epoll_disable(AioContext *ctx) >>> { >>> - ctx->epoll_available = false; >>> - if (!ctx->epoll_enabled) { >>> + ctx->epoll_enabled = false; >>> + if (!ctx->epoll_available) { >>> return; >>> } >>> - ctx->epoll_enabled = false; >>> + ctx->epoll_available = false; >>> close(ctx->epollfd); >>> } >>> >>> @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) >>> void aio_context_destroy(AioContext *ctx) >>> { >>> #ifdef CONFIG_EPOLL_CREATE1 >>> - if (ctx->epollfd >= 0) { >>> - close(ctx->epollfd); >>> - } >>> + aio_epoll_disable(ctx); >> >> Hmm... I think this patch should be the first if to split. >> >> Anyway, IMHO version 5 is already good enough and has got r-bs, no >> need to bother reposting a version 7. Maintainer could possibly touch >> up the commit message if necessary. >> >> Thanks, >> >>> #endif >>> } >>> >>> -- >>> 1.8.3.1 >>> >>
On Fri, 05/18 16:24, WangJie (Pluto) wrote:
> Ping
I'll send a pull request with v5.
Fam
diff --git a/util/aio-posix.c b/util/aio-posix.c index 0ade2c7..118bf57 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -45,11 +45,11 @@ struct AioHandler static void aio_epoll_disable(AioContext *ctx) { - ctx->epoll_available = false; - if (!ctx->epoll_enabled) { + ctx->epoll_enabled = false; + if (!ctx->epoll_available) { return; } - ctx->epoll_enabled = false; + ctx->epoll_available = false; close(ctx->epollfd); } @@ -716,9 +716,7 @@ void aio_context_setup(AioContext *ctx) void aio_context_destroy(AioContext *ctx) { #ifdef CONFIG_EPOLL_CREATE1 - if (ctx->epollfd >= 0) { - close(ctx->epollfd); - } + aio_epoll_disable(ctx); #endif }
epoll_available will only be set if epollfd != -1, os we can swap the two variables in aio_epoll_disable, and aio_context_destroy can call aio_epoll_disable directly. Signed-off-by: Jie Wang <wangjie88@huawei.com> --- util/aio-posix.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)