qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 1/5] qemu-nbd: Add --pid-file option
Date: Tue, 7 May 2019 14:30:48 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1

On 5/7/19 1:36 PM, 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 <address@hidden>
> ---
>  qemu-nbd.c    | 29 +++++++++++++++++++++++++++++
>  qemu-nbd.texi |  2 ++
>  2 files changed, 31 insertions(+)
> 

> @@ -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"

Should --pid-file imply --fork, or be an error if --fork was not
supplied? As coded, it writes a pid file regardless of --fork, even
though it is less obvious that it is useful in that case. I don't have a
strong preference (there doesn't seem to be a useful consensus on what
forking daemons should do), but it would at least be worth documenting
the intended action (even if that implies a tweak to the patch to match
the intent).

>  #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;

Bikeshedding: pid_name is nicer (path makes me think of $PATH and other
colon-separated lists, which this is not).

Otherwise, I agree that this is long overdue. Thanks! If you can justify
the behavior without --fork,
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]