On 2014/1/23 8:27, Serge Hallyn wrote: > Quoting Qiang Huang (h.huangqi...@huawei.com): >> When start container with daemon model, we'll have a new daemon >> process in lxcapi_start, whose c->numthreads is 2, inherited >> from his father, and his father's return and lxc_container_put >> won't affect son's numthreads. So when daemon stops, he only >> did on lxc_container_put, the lxc_container will leak. >> >> Actually, lxc_container_get only works for mulit-threads cases, >> here we did fork, and don't need to hold container count to >> protect anything, so just remove it. >> >> Signed-off-by: Serge Hallyn <serge.hal...@ubuntu.com> > > No,
Sorry, I'll drop this. > >> Signed-off-by: Qiang Huang <h.huangqi...@huawei.com> >> --- >> src/lxc/lxccontainer.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c >> index 368cb46..d020918 100644 >> --- a/src/lxc/lxccontainer.c >> +++ b/src/lxc/lxccontainer.c >> @@ -589,15 +589,11 @@ static bool lxcapi_start(struct lxc_container *c, int >> useinit, char * const argv >> * while container is running... >> */ >> if (daemonize) { >> - if (!lxc_container_get(c)) >> - return false; >> lxc_monitord_spawn(c->config_path); >> >> pid_t pid = fork(); >> - if (pid < 0) { >> - lxc_container_put(c); >> + if (pid < 0) >> return false; >> - } >> if (pid != 0) >> return wait_on_daemonized_start(c, pid); >> >> @@ -639,6 +635,10 @@ reboot: >> } >> >> if (daemonize) { >> + /* When daemon was forked, it inherited parent's >> + * lxc_container, so here need a put to free >> + * lxc_container. >> + */ >> lxc_container_put(c); > > Why did you change this in my patch? Since we have removed the > lxc_container_get() at the top of this patch, the matching put > should also be removed. > > I'm going to go ahead an apply my original patch > (https://lists.linuxcontainers.org/pipermail/lxc-devel/2014-January/007343.html) > If you think there is an error in that logic, it'll be easier > to reason about as a separate patch on top of mine. I already reasoned in the added comment, we need this because if not, lxc_container won't be freed when daemon exits, and PID file won't be unlinked either. What about this: >From 80f3862f9c4dbc8a05e79e50c50e79e30ffebc25 Mon Sep 17 00:00:00 2001 From: Qiang Huang <h.huangqi...@huawei.com> Date: Thu, 23 Jan 2014 14:25:31 +0800 Subject: [PATCH] daemon: add lxc_container_put to free container when daemon exits PID file in lxc_container is unlinked when lxc_container_free, if we leak the container, the PID file also won't be removed after container down. Signed-off-by: Qiang Huang <h.huangqi...@huawei.com> --- src/lxc/lxccontainer.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c index 28de455..d76e386 100644 --- a/src/lxc/lxccontainer.c +++ b/src/lxc/lxccontainer.c @@ -669,9 +669,14 @@ reboot: goto reboot; } - if (daemonize) + if (daemonize) { + /* When daemon forked, he inherited father's + * lxc_container, so here need a put to free + * lxc_container. + */ + lxc_container_put(c); exit (ret == 0 ? true : false); - else + } else return (ret == 0 ? true : false); } -- 1.8.3 > >> exit (ret == 0 ? true : false); >> } else { >> -- >> 1.8.3 >> >> > > . > _______________________________________________ lxc-devel mailing list lxc-devel@lists.linuxcontainers.org http://lists.linuxcontainers.org/listinfo/lxc-devel