Re: [Qemu-trivial] [Qemu-devel] [PATCH v2] vl.c: move pidfile creation up the line

[Markus Armbruster <armbru@redhat.com>]

Michael Tokarev <mjt@tls.msk.ru> writes:

> With current code, pid file is open after various
> sockets, chardevs, fsdevs and the like.  This causes
> interesting effects, for example when monitor is a
> unix-socket, and another qemu instance is already
> running, new qemu first "damages" the socket and
> next complain that it can't acquire the pid file and
> exits, making running qemu unreachable.
>
> Move pid file creation earlier, right after the call
> to os_daemonize(), where we know our process id (pid).
>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  vl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> v2: move the pid file creation even earlier, as per
> comment by Markus Armbruster
>
> diff --git a/vl.c b/vl.c
> index 368510f..ce7e998 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4058,6 +4058,11 @@ int main(int argc, char **argv, char **envp)
>  
>      os_daemonize();
>  
> +    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> +        error_report("could not acquire pid file: %s", strerror(errno));
> +        exit(1);
> +    }
> +
>      if (qemu_init_main_loop(&main_loop_err)) {
>          error_report_err(main_loop_err);
>          exit(1);
> @@ -4335,11 +4340,6 @@ int main(int argc, char **argv, char **envp)
>      }
>  #endif
>  
> -    if (pid_file && qemu_create_pidfile(pid_file) != 0) {
> -        error_report("could not acquire pid file: %s", strerror(errno));
> -        exit(1);
> -    }
> -
>      if (qemu_opts_foreach(qemu_find_opts("device"),
>                            device_help_func, NULL, NULL)) {
>          exit(0);

Right after os_daemonize() is as early as possible.  There might be
stuff happening before os_daemonize() that shouldn't, but I'm not
demanding you search for it to get this patch in.

The wider problem: we spread around parsing, checking and acting upon
command line options pretty carelessly, even though checking after
daemonize and acting before it is problematic.

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>