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;
}
