Re: [Qemu-devel] [PATCH 2/6] qemu-ga: avoid unconditional lockfile file descriptor leak

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On Wed, May 16, 2012 at 03:07:57PM +0200, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> Do not leak a file descriptor.
> Also, do not forget to unlink the lockfile upon failed lockf.
> Always close the lockfile file descriptor, taking care
> to diagnose close, as well as open and write, failure.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  qemu-ga.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 680997e..6c6de55 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -241,12 +241,13 @@ void ga_set_response_delimited(GAState *s)
>  static bool ga_open_pidfile(const char *pidfile)
>  {
>      int pidfd;
> +    int write_err;
>      char pidstr[32];
> 
>      pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
>      if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
>          g_critical("Cannot lock pid file, %s", strerror(errno));
> -        return false;
> +        goto fail;
>      }
> 
>      if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
> @@ -254,7 +255,9 @@ static bool ga_open_pidfile(const char *pidfile)
>          goto fail;
>      }
>      sprintf(pidstr, "%d", getpid());
> -    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> +    write_err = write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr);
> +    if (close(pidfd) || write_err) {

Closing the fd gives up the lock, which is not what we want in this
case. The intention is to hold it for the life of the process. In the
case of close() failing the error msg will also be inaccurate.

So I think we can leave this chunk out. In the error case the fd will get
cleaned up with the handling you added below.

For the success case, If we want to avoid leaking it, I would suggest
assigning it to a new field in GAState which is close()'d somewhere in
main() after we exit from g_main_loop_run().

> +        pidfd = -1;
>          g_critical("Failed to write pid file");
>          goto fail;
>      }
> @@ -262,6 +265,9 @@ static bool ga_open_pidfile(const char *pidfile)
>      return true;
> 
>  fail:
> +    if (pidfd != -1) {
> +        close(pidfd);

Since the patch wants to report close() errors above, we should do it
here too.

> +    }
>      unlink(pidfile);
>      return false;
>  }
> -- 
> 1.7.10.2.520.g6a4a482
>