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

[Max Reitz]

On 07.05.19 21:30, Eric Blake wrote:
> 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).

I think the documentation is pretty clear.  It stores the server's PID,
whether it has been forked or not.

I don't think we would gain anything from forbidding --pid-file without
--fork, would we?

>>  #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).

I'd prefer pid_filename myself, then, because pid_name sounds like a
weird way to say "process name". O:-)

> Otherwise, I agree that this is long overdue. Thanks! If you can justify
> the behavior without --fork,

I just can’t think of a reason not to allow it without --fork.  Maybe a
user doesn’t need --fork because they just start the server in the
background and that’s good enough, but they still want a PID file.  So
basically like common.rc’s _qemu_nbd_wrapper() before this series.

Max

signature.asc
Description: OpenPGP digital signature