From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44802) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RixWE-0008OU-4S for qemu-devel@nongnu.org; Thu, 05 Jan 2012 19:18:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RixWC-0001ew-T2 for qemu-devel@nongnu.org; Thu, 05 Jan 2012 19:18:50 -0500 Received: from e38.co.us.ibm.com ([32.97.110.159]:55080) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RixWC-0001ep-MD for qemu-devel@nongnu.org; Thu, 05 Jan 2012 19:18:48 -0500 Received: from /spool/local by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 5 Jan 2012 17:18:46 -0700 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q060ITau117788 for ; Thu, 5 Jan 2012 17:18:29 -0700 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q060IRbT009808 for ; Thu, 5 Jan 2012 17:18:28 -0700 Message-ID: <4F063DD2.3080104@linux.vnet.ibm.com> Date: Thu, 05 Jan 2012 18:18:26 -0600 From: Michael Roth MIME-Version: 1.0 References: <4F062390.6070007@intellilink.co.jp> In-Reply-To: <4F062390.6070007@intellilink.co.jp> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] GuestAgent: PIDFILE remains when daemon start fails List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "MATSUDA, Daiki" Cc: Kazuo Tanaka , qemu-devel@nongnu.org, Luiz Capitulino 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; > }