Re: [Qemu-devel] [PATCH] pr-helper: Rework socket path handling

[Paolo Bonzini]

On 03/07/2018 11:51, Michal Privoznik wrote:
> When reviewing Paolo's pr-helper patches I've noticed couple of
> problems:
> 
> 1) socket_path needs to be calculated at two different places
> (one for printing out help, the other if socket activation is NOT
> used),
> 
> 2) even though the default socket_path is allocated in
> compute_default_paths() it is the only default path the function
> handles. For instance, pidfile is allocated outside of this
> function. And yet again, at different places than 1)
> 
> Signed-off-by: Michal Privoznik <address@hidden>

Queued, thanks.

Paolo

> ---
>  scsi/qemu-pr-helper.c | 36 ++++++++++--------------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
> index 0218d65bbf..59df3fd608 100644
> --- a/scsi/qemu-pr-helper.c
> +++ b/scsi/qemu-pr-helper.c
> @@ -76,14 +76,12 @@ static int gid = -1;
>  
>  static void compute_default_paths(void)
>  {
> -    if (!socket_path) {
> -        socket_path = 
> qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> -    }
> +    socket_path = qemu_get_local_state_pathname("run/qemu-pr-helper.sock");
> +    pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
>  }
>  
>  static void usage(const char *name)
>  {
> -    compute_default_paths();
>      (printf) (
>  "Usage: %s [OPTIONS] FILE\n"
>  "Persistent Reservation helper program for QEMU\n"
> @@ -844,19 +842,6 @@ static gboolean accept_client(QIOChannel *ioc, 
> GIOCondition cond, gpointer opaqu
>      return TRUE;
>  }
>  
> -
> -/*
> - * Check socket parameters compatibility when socket activation is used.
> - */
> -static const char *socket_activation_validate_opts(void)
> -{
> -    if (socket_path != NULL) {
> -        return "Unix socket can't be set when using socket activation";
> -    }
> -
> -    return NULL;
> -}
> -
>  static void termsig_handler(int signum)
>  {
>      atomic_cmpxchg(&state, RUNNING, TERMINATE);
> @@ -930,6 +915,7 @@ int main(int argc, char **argv)
>      char *trace_file = NULL;
>      bool daemonize = false;
>      bool pidfile_specified = false;
> +    bool socket_path_specified = false;
>      unsigned socket_activation;
>  
>      struct sigaction sa_sigterm;
> @@ -946,12 +932,14 @@ int main(int argc, char **argv)
>      qemu_add_opts(&qemu_trace_opts);
>      qemu_init_exec_dir(argv[0]);
>  
> -    pidfile = qemu_get_local_state_pathname("run/qemu-pr-helper.pid");
> +    compute_default_paths();
>  
>      while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
>          switch (ch) {
>          case 'k':
> -            socket_path = optarg;
> +            g_free(socket_path);
> +            socket_path = g_strdup(optarg);
> +            socket_path_specified = true;
>              if (socket_path[0] != '/') {
>                  error_report("socket path must be absolute");
>                  exit(EXIT_FAILURE);
> @@ -1042,10 +1030,9 @@ int main(int argc, char **argv)
>      socket_activation = check_socket_activation();
>      if (socket_activation == 0) {
>          SocketAddress saddr;
> -        compute_default_paths();
>          saddr = (SocketAddress){
>              .type = SOCKET_ADDRESS_TYPE_UNIX,
> -            .u.q_unix.path = g_strdup(socket_path)
> +            .u.q_unix.path = socket_path,
>          };
>          server_ioc = qio_channel_socket_new();
>          if (qio_channel_socket_listen_sync(server_ioc, &saddr, &local_err) < 
> 0) {
> @@ -1053,12 +1040,10 @@ int main(int argc, char **argv)
>              error_report_err(local_err);
>              return 1;
>          }
> -        g_free(saddr.u.q_unix.path);
>      } else {
>          /* Using socket activation - check user didn't use -p etc. */
> -        const char *err_msg = socket_activation_validate_opts();
> -        if (err_msg != NULL) {
> -            error_report("%s", err_msg);
> +        if (socket_path_specified) {
> +            error_report("Unix socket can't be set when using socket 
> activation");
>              exit(EXIT_FAILURE);
>          }
>  
> @@ -1075,7 +1060,6 @@ int main(int argc, char **argv)
>                           error_get_pretty(local_err));
>              exit(EXIT_FAILURE);
>          }
> -        socket_path = NULL;
>      }
>  
>      if (qemu_init_main_loop(&local_err)) {
>