| Message ID | 20191212163904.159893-85-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofs daemon [all] | expand |
> From: Liu Bo <bo.liu@linux.alibaba.com> > > For fuse's queueinfo, both queueinfo array and queueinfos are allocated in > fv_queue_set_started() but not cleaned up when the daemon process quits. > > This fixes the leak in proper places. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > --- > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index 7b22ae8d4f..a364f23d5d 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > } > close(ourqi->kill_fd); > ourqi->kick_fd = -1; > + free(vud->qi[qidx]); > + vud->qi[qidx] = NULL; > } > } > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session *se) > void virtio_session_close(struct fuse_session *se) > { > close(se->vu_socketfd); I beleve above close() should be removed as it is called 6 line below. > + > + if (!se->virtio_dev) { > + return; > + } > + > + close(se->vu_socketfd); > + free(se->virtio_dev->qi); > free(se->virtio_dev); > se->virtio_dev = NULL; > } > -- > 2.23.0
* Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > From: Liu Bo <bo.liu@linux.alibaba.com> > > > > For fuse's queueinfo, both queueinfo array and queueinfos are allocated in > > fv_queue_set_started() but not cleaned up when the daemon process quits. > > > > This fixes the leak in proper places. > > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > > --- > > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > > index 7b22ae8d4f..a364f23d5d 100644 > > --- a/tools/virtiofsd/fuse_virtio.c > > +++ b/tools/virtiofsd/fuse_virtio.c > > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > > } > > close(ourqi->kill_fd); > > ourqi->kick_fd = -1; > > + free(vud->qi[qidx]); > > + vud->qi[qidx] = NULL; > > } > > } > > > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session *se) > > void virtio_session_close(struct fuse_session *se) > > { > > close(se->vu_socketfd); > > I beleve above close() should be removed as it is called 6 line below. You're right, I think that's my fault from when I merged this patch with 'Virtiofsd: fix segfault when quit before dev init'. Fixed. Thanks. Dave > > + > > + if (!se->virtio_dev) { > > + return; > > + } > > + > > + close(se->vu_socketfd); > > + free(se->virtio_dev->qi); > > free(se->virtio_dev); > > se->virtio_dev = NULL; > > } > > -- > > 2.23.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> * Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > > From: Liu Bo <bo.liu@linux.alibaba.com> > > > > > > For fuse's queueinfo, both queueinfo array and queueinfos are > > > allocated in > > > fv_queue_set_started() but not cleaned up when the daemon process quits. > > > > > > This fixes the leak in proper places. > > > > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > > > --- > > > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/tools/virtiofsd/fuse_virtio.c > > > b/tools/virtiofsd/fuse_virtio.c index 7b22ae8d4f..a364f23d5d 100644 > > > --- a/tools/virtiofsd/fuse_virtio.c > > > +++ b/tools/virtiofsd/fuse_virtio.c > > > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > > > } > > > close(ourqi->kill_fd); > > > ourqi->kick_fd = -1; > > > + free(vud->qi[qidx]); > > > + vud->qi[qidx] = NULL; > > > } > > > } > > > > > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session > > > *se) void virtio_session_close(struct fuse_session *se) { > > > close(se->vu_socketfd); > > > > I beleve above close() should be removed as it is called 6 line below. > > You're right, I think that's my fault from when I merged this patch with 'Virtiofsd: fix segfault when quit before dev init'. > > Fixed. Given that: Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Thanks. > Thanks. > > Dave > > > > + > > > + if (!se->virtio_dev) { > > > + return; > > > + } > > > + > > > + close(se->vu_socketfd); > > > + free(se->virtio_dev->qi); > > > free(se->virtio_dev); > > > se->virtio_dev = NULL; > > > } > > > -- > > > 2.23.0 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* misono.tomohiro@fujitsu.com (misono.tomohiro@fujitsu.com) wrote: > > * Misono Tomohiro (misono.tomohiro@jp.fujitsu.com) wrote: > > > > From: Liu Bo <bo.liu@linux.alibaba.com> > > > > > > > > For fuse's queueinfo, both queueinfo array and queueinfos are > > > > allocated in > > > > fv_queue_set_started() but not cleaned up when the daemon process quits. > > > > > > > > This fixes the leak in proper places. > > > > > > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > > > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > > > > --- > > > > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > > > > 1 file changed, 9 insertions(+) > > > > > > > > diff --git a/tools/virtiofsd/fuse_virtio.c > > > > b/tools/virtiofsd/fuse_virtio.c index 7b22ae8d4f..a364f23d5d 100644 > > > > --- a/tools/virtiofsd/fuse_virtio.c > > > > +++ b/tools/virtiofsd/fuse_virtio.c > > > > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > > > > } > > > > close(ourqi->kill_fd); > > > > ourqi->kick_fd = -1; > > > > + free(vud->qi[qidx]); > > > > + vud->qi[qidx] = NULL; > > > > } > > > > } > > > > > > > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session > > > > *se) void virtio_session_close(struct fuse_session *se) { > > > > close(se->vu_socketfd); > > > > > > I beleve above close() should be removed as it is called 6 line below. > > > > You're right, I think that's my fault from when I merged this patch with 'Virtiofsd: fix segfault when quit before dev init'. > > > > Fixed. > > Given that: > Reviewed-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Thank you! Dave > Thanks. > > > Thanks. > > > > Dave > > > > > > + > > > > + if (!se->virtio_dev) { > > > > + return; > > > > + } > > > > + > > > > + close(se->vu_socketfd); > > > > + free(se->virtio_dev->qi); > > > > free(se->virtio_dev); > > > > se->virtio_dev = NULL; > > > > } > > > > -- > > > > 2.23.0 > > > > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes: > From: Liu Bo <bo.liu@linux.alibaba.com> > > For fuse's queueinfo, both queueinfo array and queueinfos are allocated in > fv_queue_set_started() but not cleaned up when the daemon process quits. > > This fixes the leak in proper places. > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > --- > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > index 7b22ae8d4f..a364f23d5d 100644 > --- a/tools/virtiofsd/fuse_virtio.c > +++ b/tools/virtiofsd/fuse_virtio.c > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > } > close(ourqi->kill_fd); > ourqi->kick_fd = -1; > + free(vud->qi[qidx]); > + vud->qi[qidx] = NULL; > } > } > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session *se) > void virtio_session_close(struct fuse_session *se) > { > close(se->vu_socketfd); > + > + if (!se->virtio_dev) { > + return; > + } > + > + close(se->vu_socketfd); > + free(se->virtio_dev->qi); > free(se->virtio_dev); > se->virtio_dev = NULL; > } There's a duplicated "close(se->vu_socketfd);" statement here. Sergio.
* Sergio Lopez (slp@redhat.com) wrote: > > Dr. David Alan Gilbert (git) <dgilbert@redhat.com> writes: > > > From: Liu Bo <bo.liu@linux.alibaba.com> > > > > For fuse's queueinfo, both queueinfo array and queueinfos are allocated in > > fv_queue_set_started() but not cleaned up when the daemon process quits. > > > > This fixes the leak in proper places. > > > > Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com> > > Signed-off-by: Eric Ren <renzhen@linux.alibaba.com> > > --- > > tools/virtiofsd/fuse_virtio.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c > > index 7b22ae8d4f..a364f23d5d 100644 > > --- a/tools/virtiofsd/fuse_virtio.c > > +++ b/tools/virtiofsd/fuse_virtio.c > > @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) > > } > > close(ourqi->kill_fd); > > ourqi->kick_fd = -1; > > + free(vud->qi[qidx]); > > + vud->qi[qidx] = NULL; > > } > > } > > > > @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session *se) > > void virtio_session_close(struct fuse_session *se) > > { > > close(se->vu_socketfd); > > + > > + if (!se->virtio_dev) { > > + return; > > + } > > + > > + close(se->vu_socketfd); > > + free(se->virtio_dev->qi); > > free(se->virtio_dev); > > se->virtio_dev = NULL; > > } > > There's a duplicated "close(se->vu_socketfd);" statement here. Yep, already spotted/fixed: https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg02901.html Dave > Sergio. -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c index 7b22ae8d4f..a364f23d5d 100644 --- a/tools/virtiofsd/fuse_virtio.c +++ b/tools/virtiofsd/fuse_virtio.c @@ -671,6 +671,8 @@ static void fv_queue_set_started(VuDev *dev, int qidx, bool started) } close(ourqi->kill_fd); ourqi->kick_fd = -1; + free(vud->qi[qidx]); + vud->qi[qidx] = NULL; } } @@ -878,6 +880,13 @@ int virtio_session_mount(struct fuse_session *se) void virtio_session_close(struct fuse_session *se) { close(se->vu_socketfd); + + if (!se->virtio_dev) { + return; + } + + close(se->vu_socketfd); + free(se->virtio_dev->qi); free(se->virtio_dev); se->virtio_dev = NULL; }