Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main()

[Li Qiang]

Alex Chen <alex.chen@huawei.com> 于2020年11月26日周四 下午7:40写道：
>
> On 2020/11/26 18:50, Li Qiang wrote:
> > Alex Chen <alex.chen@huawei.com>
> >>
> >> 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.
> >> When -s is used, a new socket fd is created, and this socket fd
> >> needs to be closed at the end of main().
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> >> ---
> >>  fsdev/virtfs-proxy-helper.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> >> index 15c0e79b06..339d477169 100644
> >> --- a/fsdev/virtfs-proxy-helper.c
> >> +++ b/fsdev/virtfs-proxy-helper.c
> >> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv)
> >>      process_requests(sock);
> >>  error:
> >>      g_free(rpath);
> >> +    if (sock_name) {
> >> +        close(sock);
> >> +    }
> >
> > If 'proxy_socket' failed, you call close(-1).
> >
> > Maybe following is better?
> >
> > if (sock >= 0) {
> >     close(sock);
> > }
> >
>
> Hi Qiang,
>
> Thanks for your review.
> The 'sock' need to be closed only when option -s is used, that is when 
> 'sock_name' is not NULL.
> So maybe the following is better?

Yes, you're right.


>
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index 15c0e79b06..3ba68d9878 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -1154,6 +1154,9 @@ int main(int argc, char **argv)
>      process_requests(sock);
>  error:
>      g_free(rpath);
> +    if (sock_name && (sock >= 0)) {

No need parenthesis for 'sock>=0'?

Thanks,
Li Qiang

> +        close(sock);
> +    }
>      g_free(sock_name);
>      do_log(LOG_INFO, "Done\n");
>      closelog();
>
> Thanks,
> Alex
>