Re: [Qemu-devel] [PATCH] GuestAgent: PIDFILE remains when daemon start fails

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

On 01/05/2012 04:26 PM, MATSUDA, Daiki wrote:
> Hi, all.
>
> I am trying QEMU Guest Agent and encountered a small bug. It is that the
> PIDFILE remains when daemon start fails. And maybe forgotton to g_free().
>
> MATSUDA, Daiki
>

Thanks for the patch. There was some contention in the past about 
whether or not to clean up pidfiles when there was abnormal termination, 
but personally I like this approach better. Luiz?:

http://www.mail-archive.com/qemu-devel@nongnu.org/msg67889.html

Patch looks good. We're missing some cleanup (gio channels, g_main_loop, 
etc.), but that can be added later, and some of those make more sense to 
have in main(). One suggestion below though:

>
>
> diff -uNrp qemu/qemu-ga.c qemu-c47f3223658119219bbe0b8d09da733d1c06e76f/qemu-ga.c
> --- qemu/qemu-ga.c	2012-01-05 01:06:25.000000000 +0900
> +++ qemu-c47f3223658119219bbe0b8d09da733d1c06e76f/qemu-ga.c	2012-01-06 07:07:03.807872085 +0900
> @@ -49,6 +49,13 @@ struct GAState {
>  };
>
>  static struct GAState *ga_state;
> +const char *pidfile = QGA_PIDFILE_DEFAULT;
> +
> +static void cleanup(void)
> +{
> +    g_free(ga_state);
> +    unlink(pidfile);
> +}
>
>  static void quit_handler(int sig)
>  {
> @@ -70,6 +77,7 @@ static void register_signal_handlers(voi
>      ret = sigaction(SIGINT, &sigact, NULL);
>      if (ret == -1) {
>          g_error("error configuring signal handler: %s", strerror(errno));
> +        cleanup();
>          exit(EXIT_FAILURE);
>      }
>      ret = sigaction(SIGTERM, &sigact, NULL);
> @@ -485,6 +493,7 @@ static void init_guest_agent(GAState *s)
>      if (s->path == NULL) {
>          if (strcmp(s->method, "virtio-serial") != 0) {
>              g_critical("must specify a path for this channel");
> +            cleanup();

we should probably just do a "return false;" or something here, check 
for return value in main(), and call cleanup()/exit() there. Looks a 
little nicer at least, and make it easier to determine when to cleanup 
(hard to tell whether init_guest_agent() was run before/after pidfile 
creation here, for instance, but obvious in main()). Same with 
register_signal_handlers() actually.

>              exit(EXIT_FAILURE);
>          }
>          /* try the default path for the virtio-serial port */
> @@ -496,17 +505,20 @@ static void init_guest_agent(GAState *s)
>          fd = qemu_open(s->path, O_RDWR | O_NONBLOCK | O_ASYNC);
>          if (fd == -1) {
>              g_critical("error opening channel: %s", strerror(errno));
> +            cleanup();
>              exit(EXIT_FAILURE);
>          }
>          ret = conn_channel_add(s, fd);
>          if (ret) {
>              g_critical("error adding channel to main loop");
> +            cleanup();
>              exit(EXIT_FAILURE);
>          }
>      } else if (strcmp(s->method, "isa-serial") == 0) {
>          fd = qemu_open(s->path, O_RDWR | O_NOCTTY);
>          if (fd == -1) {
>              g_critical("error opening channel: %s", strerror(errno));
> +            cleanup();

same here. etc.

>              exit(EXIT_FAILURE);
>          }
>          tcgetattr(fd, &tio);
> @@ -533,15 +545,18 @@ static void init_guest_agent(GAState *s)
>          fd = unix_listen(s->path, NULL, strlen(s->path));
>          if (fd == -1) {
>              g_critical("error opening path: %s", strerror(errno));
> +            cleanup();
>              exit(EXIT_FAILURE);
>          }
>          ret = listen_channel_add(s, fd, true);
>          if (ret) {
>              g_critical("error binding/listening to specified socket");
> +            cleanup();
>              exit(EXIT_FAILURE);
>          }
>      } else {
>          g_critical("unsupported channel method/type: %s", s->method);
> +        cleanup();
>          exit(EXIT_FAILURE);
>      }
>
> @@ -552,7 +567,7 @@ static void init_guest_agent(GAState *s)
>  int main(int argc, char **argv)
>  {
>      const char *sopt = "hVvdm:p:l:f:b:";
> -    const char *method = NULL, *path = NULL, *pidfile = QGA_PIDFILE_DEFAULT;
> +    const char *method = NULL, *path = NULL;
>      const struct option lopt[] = {
>          { "help", 0, NULL, 'h' },
>          { "version", 0, NULL, 'V' },
> @@ -662,7 +677,7 @@ int main(int argc, char **argv)
>      g_main_loop_run(ga_state->main_loop);
>
>      ga_command_state_cleanup_all(ga_state->command_state);
> -    unlink(pidfile);
> +    cleanup();
>
>      return 0;
>  }