Re: [Qemu-devel] [PATCH 1/5] qemu-nbd: Add --pid-file option

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, May 07, 2019 at 08:36:06PM +0200, Max Reitz wrote:
> --fork is a bit boring if there is no way to get the child's PID.  This
> option helps.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-nbd.c    | 29 +++++++++++++++++++++++++++++
>  qemu-nbd.texi |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index dca9e72cee..7e48154f44 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -59,6 +59,7 @@
>  #define QEMU_NBD_OPT_IMAGE_OPTS    262
>  #define QEMU_NBD_OPT_FORK          263
>  #define QEMU_NBD_OPT_TLSAUTHZ      264
> +#define QEMU_NBD_OPT_PID_FILE      265
>  
>  #define MBR_SIZE 512
>  
> @@ -111,6 +112,7 @@ static void usage(const char *name)
>  "                            specify tracing options\n"
>  "  --fork                    fork off the server process and exit the parent\n"
>  "                            once the server is running\n"
> +"  --pid-file=PATH           store the server's process ID in the given file\n"
>  #if HAVE_NBD_DEVICE
>  "\n"
>  "Kernel NBD client support:\n"
> @@ -651,6 +653,7 @@ int main(int argc, char **argv)
>          { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
>          { "trace", required_argument, NULL, 'T' },
>          { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK },
> +        { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE },
>          { NULL, 0, NULL, 0 }
>      };
>      int ch;
> @@ -677,6 +680,8 @@ int main(int argc, char **argv)
>      bool list = false;
>      int old_stderr = -1;
>      unsigned socket_activation;
> +    const char *pid_path = NULL;
> +    FILE *pid_file;
>  
>      /* The client thread uses SIGTERM to interrupt the server.  A signal
>       * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
> @@ -876,6 +881,9 @@ int main(int argc, char **argv)
>          case 'L':
>              list = true;
>              break;
> +        case QEMU_NBD_OPT_PID_FILE:
> +            pid_path = optarg;
> +            break;
>          }
>      }
>  
> @@ -1196,6 +1204,27 @@ int main(int argc, char **argv)
>  
>      nbd_update_server_watch();
>  
> +    if (pid_path) {
> +        pid_file = fopen(pid_path, "w");
> +        if (!pid_file) {
> +            error_report("Failed to store PID in %s: %s",
> +                         pid_path, strerror(errno));
> +            exit(EXIT_FAILURE);
> +        }
> +
> +        ret = fprintf(pid_file, "%ld", (long)getpid());
> +        if (ret < 0) {
> +            ret = -errno;
> +        }
> +        fclose(pid_file);
> +
> +        if (ret < 0) {
> +            error_report("Failed to store PID in %s: %s",
> +                         pid_path, strerror(-ret));
> +            exit(EXIT_FAILURE);
> +        }
> +    }

This is racy because multiple qemu-nbd's can be started pointing to
the same pidfile and one will blindly overwrite the other.

Please use  qemu_write_pidfile instead which acquires locks on the
pidfile in a race free manner.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|