On Thu, 26 Nov 2020 18:52:39 +0100 Christian Schoenebeck <qemu_...@crudebyte.com> wrote:
> On Donnerstag, 26. November 2020 14:15:51 CET Alex Chen wrote: > > Hi Greg, > > > > Thanks for your review. > > > > On 2020/11/26 20:07, Greg Kurz wrote: > > > On Thu, 26 Nov 2020 10:16:24 +0000 > > > > > > Alex Chen <alex.c...@huawei.com> wrote: > > >> Only one of the options -s and -f can be used. When -f is used, > > >> the fd is created externally and does not need to be closed. > > So somebody is really using the 9p proxy driver for something; interesting. > > > > > > > The process running virtfs-proxy-helper has its own copy of > > > the fd inherited from its parent. And this fd will be closed > > > eventually when the process terminates. > > > > > >> When -s is used, a new socket fd is created, and this socket fd > > >> needs to be closed at the end of main(). > > > > > > Same here, the new socket fd is closed when the process > > > terminates. > > Does it? I haven't reviewed much of the 9p proxy code yet, however if > chroot() > fails for instance, the fd would leak right now, wouldn't it? > This is done just at the end of main()... the leak won't last long. > Or was your argument that it's the OS's job to free any file descriptor > automatically on process terminations in general? > That's exactly my point. The only justification that'd deserve to be in the changelog of such a patch is something like "because this is good practice to rollback in case code moves to another function than main()". > > IMO, it's best to explicitly release resources before the process > > terminates, just as the variable 'rpath' is explicitly freed in main(), > > so socket fd also needs to be explicitly closed here. > > > > Looking forward to your reply. > > > > > The only justification to merge such a change would be if > > > the code was sitting in some other function, in which > > > case we should indeed do proper rollback. But it is main() > > > here, so this patch isn't needed. > > > > > >> Reported-by: Euler Robot <euler.ro...@huawei.com> > > > > > > Can you provide a copy of the report in case I'm > > > missing something ? > > > > Our codecheck tool reports a resource leak here, which is relatively simple, > > like the one below, I did not attach it. > > > > --------------------- > > "Resource leak: sock" > > --------------------- > > Yeah, not very helpful that output. > Indeed :D > > > > Thanks, > > Alex > > Best regards, > Christian Schoenebeck > >
