| Message ID | 20190507183610.9848-2-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Let 233 run concurrently | expand |
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>
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?
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 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
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
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
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
--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(+)