Re: [Qemu-devel] [PATCH 08/12] qga: move agent run in a seperate function

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Marc-André Lureau (2015-07-01 06:47:43)
> Once the options are populated, move the running state to
> a run_agent() function.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@gmail.com>
> ---
>  qga/main.c | 123 +++++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 67 insertions(+), 56 deletions(-)
> 
> diff --git a/qga/main.c b/qga/main.c
> index 5575637..aaf0e10 100644
> --- a/qga/main.c
> +++ b/qga/main.c
> @@ -1042,39 +1042,13 @@ static void option_parse(int argc, char **argv)
>      }
>  }
> 
> -int main(int argc, char **argv)
> +static int run_agent(GAState *s)
>  {
> -    GAState *s;
> -
> -    module_call_init(MODULE_INIT_QAPI);
> -
> -    init_dfl_pathnames();
> -    option_parse(argc, argv);
> -
> -    if (pid_filepath == NULL) {
> -        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> -    }
> -
> -    if (state_dir == NULL) {
> -        state_dir = g_strdup(dfl_pathnames.state_dir);
> -    }
> -
> -    if (method == NULL) {
> -        method = g_strdup("virtio-serial");
> -    }
> +    ga_state = s;
> 
> -    if (device_path == NULL) {
> -        if (strcmp(method, "virtio-serial") == 0) {
> -            /* try the default path for the virtio-serial port */
> -            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
> -        } else if (strcmp(method, "isa-serial") == 0) {
> -            /* try the default path for the serial port - COM1 */
> -            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
> -        } else {
> -            g_critical("must specify a path for this channel");
> -            goto out_bad;
> -        }
> -    }
> +    g_log_set_default_handler(ga_log, s);
> +    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> +    ga_enable_logging(s);
> 
>  #ifdef _WIN32
>      /* On win32 the state directory is application specific (be it the default
> @@ -1090,20 +1064,6 @@ int main(int argc, char **argv)
>      }
>  #endif
> 
> -    s = g_malloc0(sizeof(GAState));
> -    s->log_level = log_level;
> -    s->log_file = stderr;
> -#ifdef CONFIG_FSFREEZE
> -    s->fsfreeze_hook = fsfreeze_hook;
> -#endif
> -    g_log_set_default_handler(ga_log, s);
> -    g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> -    ga_enable_logging(s);
> -    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> -                                                 state_dir);
> -    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> -    s->frozen = false;
> -
>  #ifndef _WIN32
>      /* check if a previous instance of qemu-ga exited with filesystems' state
>       * marked as frozen. this could be a stale value (a non-qemu-ga process
> @@ -1154,7 +1114,7 @@ int main(int argc, char **argv)
>              if (!log_file) {
>                  g_critical("unable to open specified log file: %s",
>                             strerror(errno));
> -                goto out_bad;
> +                return EXIT_FAILURE;
>              }
>              s->log_file = log_file;
>          }
> @@ -1165,7 +1125,7 @@ int main(int argc, char **argv)
>                                 s->pstate_filepath,
>                                 ga_is_frozen(s))) {
>          g_critical("failed to load persistent state");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
> 
>      blacklist = ga_command_blacklist_init(blacklist);
> @@ -1185,14 +1145,14 @@ int main(int argc, char **argv)
>  #ifndef _WIN32
>      if (!register_signal_handlers()) {
>          g_critical("failed to register signal handlers");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
>  #endif
> 
>      s->main_loop = g_main_loop_new(NULL, false);
>      if (!channel_init(ga_state, method, device_path)) {
>          g_critical("failed to initialize guest agent channel");
> -        goto out_bad;
> +        return EXIT_FAILURE;
>      }
>  #ifndef _WIN32
>      g_main_loop_run(ga_state->main_loop);
> @@ -1206,15 +1166,65 @@ int main(int argc, char **argv)
>      }
>  #endif
> 
> -    ga_command_state_cleanup_all(ga_state->command_state);
> -    ga_channel_free(ga_state->channel);
> +    return EXIT_SUCCESS;
> +}
> 
> -    if (daemonize) {
> -        unlink(pid_filepath);
> +int main(int argc, char **argv)
> +{
> +    int ret = EXIT_SUCCESS;
> +    GAState *s = g_new0(GAState, 1);
> +
> +    module_call_init(MODULE_INIT_QAPI);
> +
> +    init_dfl_pathnames();
> +    option_parse(argc, argv);
> +
> +    if (pid_filepath == NULL) {
> +        pid_filepath = g_strdup(dfl_pathnames.pidfile);
> +    }
> +
> +    if (state_dir == NULL) {
> +        state_dir = g_strdup(dfl_pathnames.state_dir);
> +    }
> +
> +    if (method == NULL) {
> +        method = g_strdup("virtio-serial");
> +    }
> +
> +    if (device_path == NULL) {
> +        if (strcmp(method, "virtio-serial") == 0) {
> +            /* try the default path for the virtio-serial port */
> +            device_path = g_strdup(QGA_VIRTIO_PATH_DEFAULT);
> +        } else if (strcmp(method, "isa-serial") == 0) {
> +            /* try the default path for the serial port - COM1 */
> +            device_path = g_strdup(QGA_SERIAL_PATH_DEFAULT);
> +        } else {
> +            g_critical("must specify a path for this channel");
> +            ret = EXIT_FAILURE;
> +            goto end;
> +        }
> +    }
> +
> +    s->log_level = log_level;
> +    s->log_file = stderr;
> +#ifdef CONFIG_FSFREEZE
> +    s->fsfreeze_hook = fsfreeze_hook;
> +#endif
> +    s->state_filepath_isfrozen = g_strdup_printf("%s/qga.state.isfrozen",
> +                                                 state_dir);
> +    s->pstate_filepath = g_strdup_printf("%s/qga.state", state_dir);
> +    s->frozen = false;

It's hard to draw the line between what should be in main() as opposed
to run_agent(), but we have some s->frozen initialization happening
here, and then the statefile-based setting s->frozen values being set
in run_agent(), so at least in this case we should move those back to
main(). That means we probabably have to move the checks for the
statefile existence/creation back to main() as well since s->frozen
depends on it.

I think that's the right place anyway, makes sense to load/init the
persistent state file in the same location we handle the config file.

> +
> +    ret = run_agent(s);
> +
> +end:
> +    if (s->command_state) {
> +        ga_command_state_cleanup_all(s->command_state);
> +    }
> +    if (s->channel) {
> +        ga_channel_free(s->channel);
>      }
> -    return 0;
> 
> -out_bad:
>      if (daemonize) {
>          unlink(pid_filepath);
>      }
> @@ -1227,6 +1237,7 @@ out_bad:
>  #ifdef CONFIG_FSFREEZE
>      g_free(fsfreeze_hook);
>  #endif
> +    g_free(s);
> 
> -    return EXIT_FAILURE;
> +    return ret;
>  }
> -- 
> 2.4.3
>