As I already clarified in the cover letter, I will fix the typo in the
commit message and drop the message ID from all the patches, changes will
be in the next version.
On Thu, Sep 27, 2018 at 2:44 PM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:
> Hi
>
> On Thu, Sep 27, 2018 at 11:37 AM Bishara AbuHattoum <bish...@daynix.com>
> wrote:
> >
> > From: Michael Roth <mdr...@linux.vnet.ibm.com>
> >
> > This patch better separates the init/cleanup routines out into
> > separate functions to make make the start-up procedure a bit easier
>
> "make make"
>
> > to follow. This will be useful when we eventually break out the
> > actual start/stop of the agent's main loop into separates routines
> > that can be called multiple times after the init phase.
> >
> > Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> > Message-Id: <20171026233054.21133-2-mdr...@linux.vnet.ibm.com>
>
> I don't know if it make sense to retain the original Message-Id.
>
> it is not as good as it used to wrt to cleaning up on error, but that
> may be acceptable..
> Reviewed-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> > ---
> > qga/main.c | 82 +++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 50 insertions(+), 32 deletions(-)
> >
> > diff --git a/qga/main.c b/qga/main.c
> > index 6d70242d05..7b7b837a14 100644
> > --- a/qga/main.c
> > +++ b/qga/main.c
> > @@ -1245,9 +1245,21 @@ static bool check_is_frozen(GAState *s)
> > return false;
> > }
> >
> > -static int run_agent(GAState *s, GAConfig *config, int
> socket_activation)
> > +static GAState *initialize_agent(GAConfig *config)
> > {
> > - ga_state = s;
> > + GAState *s = g_new0(GAState, 1);
> > +
> > + g_assert(ga_state == NULL);
> > +
> > + s->log_level = config->log_level;
> > + s->log_file = stderr;
> > +#ifdef CONFIG_FSFREEZE
> > + s->fsfreeze_hook = config->fsfreeze_hook;
> > +#endif
> > + s->pstate_filepath = g_strdup_printf("%s/qga.state",
> config->state_dir);
> > + s->state_filepath_isfrozen =
> g_strdup_printf("%s/qga.state.isfrozen",
> > + config->state_dir);
> > + s->frozen = check_is_frozen(s);
> >
> > g_log_set_default_handler(ga_log, s);
> > g_log_set_fatal_mask(NULL, G_LOG_LEVEL_ERROR);
> > @@ -1263,7 +1275,7 @@ static int run_agent(GAState *s, GAConfig *config,
> int socket_activation)
> > if (g_mkdir_with_parents(config->state_dir, S_IRWXU) == -1) {
> > g_critical("unable to create (an ancestor of) the state
> directory"
> > " '%s': %s", config->state_dir, strerror(errno));
> > - return EXIT_FAILURE;
> > + return NULL;
> > }
> > #endif
> >
> > @@ -1288,7 +1300,7 @@ static int run_agent(GAState *s, GAConfig *config,
> int socket_activation)
> > if (!log_file) {
> > g_critical("unable to open specified log file: %s",
> > strerror(errno));
> > - return EXIT_FAILURE;
> > + return NULL;
> > }
> > s->log_file = log_file;
> > }
> > @@ -1299,7 +1311,7 @@ static int run_agent(GAState *s, GAConfig *config,
> int socket_activation)
> > s->pstate_filepath,
> > ga_is_frozen(s))) {
> > g_critical("failed to load persistent state");
> > - return EXIT_FAILURE;
> > + return NULL;
> > }
> >
> > config->blacklist = ga_command_blacklist_init(config->blacklist);
> > @@ -1320,12 +1332,37 @@ static int run_agent(GAState *s, GAConfig
> *config, int socket_activation)
> > #ifndef _WIN32
> > if (!register_signal_handlers()) {
> > g_critical("failed to register signal handlers");
> > - return EXIT_FAILURE;
> > + return NULL;
> > }
> > #endif
> >
> > s->main_loop = g_main_loop_new(NULL, false);
> >
> > + ga_state = s;
> > + return s;
> > +}
> > +
> > +static void cleanup_agent(GAState *s)
> > +{
> > + if (s->command_state) {
> > + ga_command_state_cleanup_all(s->command_state);
> > + ga_command_state_free(s->command_state);
> > + json_message_parser_destroy(&s->parser);
> > + }
> > + if (s->channel) {
> > + ga_channel_free(s->channel);
> > + }
> > + g_free(s->pstate_filepath);
> > + g_free(s->state_filepath_isfrozen);
> > + if (s->main_loop) {
> > + g_main_loop_unref(s->main_loop);
> > + }
> > + g_free(s);
> > + ga_state = NULL;
> > +}
> > +
> > +static int run_agent(GAState *s, GAConfig *config, int
> socket_activation)
> > +{
> > if (!channel_init(ga_state, config->method, config->channel_path,
> > socket_activation ? FIRST_SOCKET_ACTIVATION_FD :
> -1)) {
> > g_critical("failed to initialize guest agent channel");
> > @@ -1349,7 +1386,7 @@ static int run_agent(GAState *s, GAConfig *config,
> int socket_activation)
> > int main(int argc, char **argv)
> > {
> > int ret = EXIT_SUCCESS;
> > - GAState *s = g_new0(GAState, 1);
> > + GAState *s;
> > GAConfig *config = g_new0(GAConfig, 1);
> > int socket_activation;
> >
> > @@ -1417,44 +1454,25 @@ int main(int argc, char **argv)
> > }
> > }
> >
> > - s->log_level = config->log_level;
> > - s->log_file = stderr;
> > -#ifdef CONFIG_FSFREEZE
> > - s->fsfreeze_hook = config->fsfreeze_hook;
> > -#endif
> > - s->pstate_filepath = g_strdup_printf("%s/qga.state",
> config->state_dir);
> > - s->state_filepath_isfrozen =
> g_strdup_printf("%s/qga.state.isfrozen",
> > - config->state_dir);
> > - s->frozen = check_is_frozen(s);
> > -
> > if (config->dumpconf) {
> > config_dump(config);
> > goto end;
> > }
> >
> > + s = initialize_agent(config);
> > + if (!s) {
> > + g_critical("error initializing guest agent");
> > + goto end;
> > + }
> > ret = run_agent(s, config, socket_activation);
> > + cleanup_agent(s);
> >
> > end:
> > - if (s->command_state) {
> > - ga_command_state_cleanup_all(s->command_state);
> > - ga_command_state_free(s->command_state);
> > - json_message_parser_destroy(&s->parser);
> > - }
> > - if (s->channel) {
> > - ga_channel_free(s->channel);
> > - }
> > - g_free(s->pstate_filepath);
> > - g_free(s->state_filepath_isfrozen);
> > -
> > if (config->daemonize) {
> > unlink(config->pid_filepath);
> > }
> >
> > config_free(config);
> > - if (s->main_loop) {
> > - g_main_loop_unref(s->main_loop);
> > - }
> > - g_free(s);
> >
> > return ret;
> > }
> > --
> > 2.17.0
> >
> >
>
>
> --
> Marc-André Lureau
>
