--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);
+ }
+ }
+
/* now when the initialization is (almost) complete, chdir("/")
* to free any busy filesystems */
if (chdir("/") < 0) {
diff --git a/qemu-nbd.texi b/qemu-nbd.texi
index de342c76b8..7f55657722 100644
--- a/qemu-nbd.texi
+++ b/qemu-nbd.texi
@@ -117,6 +117,8 @@ option; or provide the credentials needed for connecting as a client
in list mode.
@item --fork
Fork off the server process and exit the parent once the server is running.
+@item --pid-file=PATH
+Store the server's process ID in the given file.
@item --tls-authz=ID
Specify the ID of a qauthz object previously created with the
--object option. This will be used to authorize connecting users
--
2.20.1
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 :|
On 08.05.19 11:01, Daniel P. Berrangé wrote:
> 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.
Ah, nice, that makes things better and easier. :-)
Max
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 <mreitz@redhat.com>
> ---
> 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 <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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 <mreitz@redhat.com>
>> ---
>> 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
On 5/7/19 2:39 PM, Max Reitz wrote: > 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 <mreitz@redhat.com> >>> --- >>> 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? I can't think of any reason to forbid it. So it sounds like we are intentional, this writes the pid into --pid-file regardless of whether that pid can be learned by other means as well. >>> + 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:-) Works for me, even if it is longer. Do you want to respin, or just have me touch it up when folding it into my NBD tree? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 07.05.19 21:51, Eric Blake wrote: > On 5/7/19 2:39 PM, Max Reitz wrote: >> 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 <mreitz@redhat.com> >>>> --- >>>> 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? > > I can't think of any reason to forbid it. So it sounds like we are > intentional, this writes the pid into --pid-file regardless of whether > that pid can be learned by other means as well. > > >>>> + 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:-) > > Works for me, even if it is longer. Do you want to respin, or just have > me touch it up when folding it into my NBD tree? I suppose I’d prefer a respin, independently of what you make of patches 4 and 5. Max
On Tue, May 07, 2019 at 09:39:01PM +0200, Max Reitz wrote: > 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 <mreitz@redhat.com> > >> --- > >> 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? Indeed, use of --pid-file should be independant of --fork, as a mgmt app may have already forked it into the background, and merely want to get the pidfile 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 :|
© 2016 - 2026 Red Hat, Inc.