Re: [lxc-devel] [PATCH 1/3] lxc-start: fix the container leak when daemonize

2014-01-23 Thread Qiang Huang 
On 2014/1/24 1:17, Serge Hallyn wrote:
> Quoting Qiang Huang (h.huangqi...@huawei.com):
>> 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.
> 
> I see, but
> 
>> What about this:
>>
>> From 80f3862f9c4dbc8a05e79e50c50e79e30ffebc25 Mon Sep 17 00:00:00 2001
>> From: Qiang Huang 
>> 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 
> 
> I was about to ack this, but then it occurred to me that
> this seems to show that the pidfile free shouldn't be done
> through lxc_container_free().  The pidfile always gets
> written right before the reboot: label, and should simply be
> explicitly removed in the place where you are adding a
> lxc_container_put().  That way it is symmetric;  by having
> it in lxc_container_free(), it is not.
> 

You are right, please review this:

---

>From 4ed573856d9f0ec56b8e522efd40f986a0409aa1 Mon Sep 17 00:00:00 2001
From: Qiang Huang 
Date: Fri, 24 Jan 2014 11:41:27 +0800
Subject: [PATCH] lxccontainer: remove PID file after lxc_start return

Make the way symmetric. This also fix the file leak in
daemon model.

Signed-off-by: Qiang Huang 
---
 src/lxc/lxccontainer.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 1520cd3..9ebb27e 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -233,11 +233,6 @@ static void lxc_container_free(struct lxc_container *c)
free(c->config_path);
c->config_path = NULL;
}
-   if (c->pidfile) {
-   unlink(c->pidfile);
-   free(c->pidfile);
-   c->pidfile = NULL;
-   }

free(c);
 }
@@ -665,6 +660,12 @@ reboot:
goto reboot;
}

+   if (c->pidfile) {
+   unlink(c->pidfile);
+   free(c->pidfile);
+   c->pidfile = NULL;
+   }
+
if (daemonize)
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

[lxc-devel] [PATCH] cgroupfs: get rid of the redundant error info

2014-01-22 Thread Qiang Huang 

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 34bf126..9513e96 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -976,10 +976,8 @@ struct cgroup_process_info *lxc_cgroupfs_create(const char 
*name, const char *pa
/* if we didn't create the cgroup, then we have 
to make sure that
 * further cgroups will be created properly
 */
-   if (handle_cgroup_settings(mp, 
info_ptr->cgroup_path) < 0) {
-   ERROR("Could not set clone_children to 
1 for cpuset hierarchy in pre-existing cgroup.");
+   if (handle_cgroup_settings(mp, 
info_ptr->cgroup_path) < 0)
goto cleanup_from_error;
-   }

/* already existed but path component of 
pattern didn't contain '%n',
 * so this is not an error; but then we don't 
need current_entire_path
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 1/3] lxc-start: fix the container leak when daemonize

2014-01-22 Thread Qiang Huang 
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 
> 
> No,

Sorry, I'll drop this.

> 
>> Signed-off-by: Qiang Huang 
>> ---
>>  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 
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 
---
 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

[lxc-devel] [PATCH] cgroupfs: need the full path to setup cpuset

2014-01-22 Thread Qiang Huang 
Function file_exists() needs a absolute full path, but we are using
current_entire_path which is not. It will get the wrong result from
file_exists() and case Segmentation fault when we fopen a non-exist
file and try to fscanf from it.

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 4da0e07..34bf126 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -958,12 +958,20 @@ struct cgroup_process_info *lxc_cgroupfs_create(const 
char *name, const char *pa
goto cleanup_from_error;
} else if (r == 0) {
/* successfully created */
+   char *full_path = NULL;
r = lxc_grow_array((void 
***)&info_ptr->created_paths, &info_ptr->created_paths_capacity, 
info_ptr->created_paths_count + 1, 8);
if (r < 0)
goto cleanup_from_error;

info_ptr->created_paths[info_ptr->created_paths_count++] = current_entire_path;
+
+   full_path = cgroup_to_absolute_path(
+   
info_ptr->designated_mount_point,
+   current_entire_path, NULL);
+   if (!full_path)
+   goto cleanup_from_error;

setup_cpuset_if_needed(info_ptr->hierarchy->subsystems,
-   current_entire_path);
+   full_path);
+   free(full_path);
} else {
/* if we didn't create the cgroup, then we have 
to make sure that
 * further cgroups will be created properly
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] cgroup.c: clean up for handle_cgroup_settings

2014-01-22 Thread Qiang Huang 
On 2014/1/23 10:55, Serge Hallyn wrote:
> Quoting Qiang Huang (h.huangqi...@huawei.com):
>> Ping...
>>
>> Hi Serge or Stéphane,
>>
>> Did you miss this?
> 
> Patch looks good,
> 
> Acked-by: Serge E. Hallyn 
> 
> but note that the there's been a touch of churn there.  If ok with
> you I'll apply your patch with the needed changes;  if you prefer to

Feel free to adjust it.
Thanks.

> send a new patch let me know.
> 
>> On 2014/1/20 16:37, Qiang Huang wrote:
>>> Clean up the nesting if, make the logic similar for memory
>>> and cpuset, and the error message should sent from inside,
>>> for better extendibility.
>>>
>>> Signed-off-by: Qiang Huang 
>>> ---
>>>  src/lxc/cgroup.c | 53 ++---
>>>  1 file changed, 34 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
>>> index 85384fc..693db94 100644
>>> --- a/src/lxc/cgroup.c
>>> +++ b/src/lxc/cgroup.c
>>> @@ -782,10 +782,8 @@ struct cgroup_process_info *lxc_cgroupfs_create(const 
>>> char *name, const char *pa
>>>
>>> if (lxc_string_in_array("ns", (const char **)h->subsystems))
>>> continue;
>>> -   if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0) {
>>> -   ERROR("Could not set clone_children to 1 for cpuset 
>>> hierarchy in parent cgroup.");
>>> +   if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0)
>>> goto out_initial_error;
>>> -   }
>>> }
>>>
>>> /* normalize the path */
>>> @@ -2016,43 +2014,60 @@ static int handle_cgroup_settings(struct 
>>> cgroup_mount_point *mp,
>>>  {
>>> int r, saved_errno = 0;
>>> char buf[2];
>>> +   const char **subsystems = (const char **)mp->hierarchy->subsystems;
>>>
>>> /* If this is the memory cgroup, we want to enforce hierarchy.
>>>  * But don't fail if for some reason we can't.
>>>  */
>>> -   if (lxc_string_in_array("memory", (const char 
>>> **)mp->hierarchy->subsystems)) {
>>> -   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
>>> "/memory.use_hierarchy");
>>> -   if (cc_path) {
>>> -   r = lxc_read_from_file(cc_path, buf, 1);
>>> -   if (r < 1 || buf[0] != '1') {
>>> -   r = lxc_write_to_file(cc_path, "1", 1, false);
>>> -   if (r < 0)
>>> -   SYSERROR("failed to set 
>>> memory.use_hiararchy to 1; continuing");
>>> -   }
>>> +   if (lxc_string_in_array("memory", subsystems)) {
>>> +   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
>>> +   "/memory.use_hierarchy");
>>> +   if (!cc_path)
>>> +   goto cpuset;
>>> +
>>> +   r = lxc_read_from_file(cc_path, buf, 1);
>>> +   if (r == 1 && buf[0] == '1') {
>>> free(cc_path);
>>> +   goto cpuset;
>>> }
>>> +
>>> +   r = lxc_write_to_file(cc_path, "1", 1, false);
>>> +   if (r < 0)
>>> +   SYSERROR("Failed to set memory.use_hiararchy to 1; 
>>> continuing");
>>> +   free(cc_path);
>>> }
>>>
>>> /* if this is a cpuset hierarchy, we have to set cgroup.clone_children 
>>> in
>>>  * the base cgroup, otherwise containers will start with an empty 
>>> cpuset.mems
>>>  * and cpuset.cpus and then
>>>  */
>>> -   if (lxc_string_in_array("cpuset", (const char 
>>> **)mp->hierarchy->subsystems)) {
>>> -   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
>>> "/cgroup.clone_children");
>>> +cpuset:
>>> +   if (lxc_string_in_array("cpuset", subsystems)) {
>>> +   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
>>> +   "/cgroup.clone_children");
>>> if (!cc_path)
>>> -   return -1;
&g

Re: [lxc-devel] [PATCH] cgroup.c: clean up for handle_cgroup_settings

2014-01-22 Thread Qiang Huang 
Ping...

Hi Serge or Stéphane,

Did you miss this?

On 2014/1/20 16:37, Qiang Huang wrote:
> Clean up the nesting if, make the logic similar for memory
> and cpuset, and the error message should sent from inside,
> for better extendibility.
> 
> Signed-off-by: Qiang Huang 
> ---
>  src/lxc/cgroup.c | 53 ++---
>  1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
> index 85384fc..693db94 100644
> --- a/src/lxc/cgroup.c
> +++ b/src/lxc/cgroup.c
> @@ -782,10 +782,8 @@ struct cgroup_process_info *lxc_cgroupfs_create(const 
> char *name, const char *pa
> 
>   if (lxc_string_in_array("ns", (const char **)h->subsystems))
>   continue;
> - if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0) {
> - ERROR("Could not set clone_children to 1 for cpuset 
> hierarchy in parent cgroup.");
> + if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0)
>   goto out_initial_error;
> - }
>   }
> 
>   /* normalize the path */
> @@ -2016,43 +2014,60 @@ static int handle_cgroup_settings(struct 
> cgroup_mount_point *mp,
>  {
>   int r, saved_errno = 0;
>   char buf[2];
> + const char **subsystems = (const char **)mp->hierarchy->subsystems;
> 
>   /* If this is the memory cgroup, we want to enforce hierarchy.
>* But don't fail if for some reason we can't.
>*/
> - if (lxc_string_in_array("memory", (const char 
> **)mp->hierarchy->subsystems)) {
> - char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
> "/memory.use_hierarchy");
> - if (cc_path) {
> - r = lxc_read_from_file(cc_path, buf, 1);
> - if (r < 1 || buf[0] != '1') {
> - r = lxc_write_to_file(cc_path, "1", 1, false);
> - if (r < 0)
> - SYSERROR("failed to set 
> memory.use_hiararchy to 1; continuing");
> - }
> + if (lxc_string_in_array("memory", subsystems)) {
> + char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
> + "/memory.use_hierarchy");
> + if (!cc_path)
> + goto cpuset;
> +
> + r = lxc_read_from_file(cc_path, buf, 1);
> + if (r == 1 && buf[0] == '1') {
>   free(cc_path);
> + goto cpuset;
>   }
> +
> + r = lxc_write_to_file(cc_path, "1", 1, false);
> + if (r < 0)
> + SYSERROR("Failed to set memory.use_hiararchy to 1; 
> continuing");
> + free(cc_path);
>   }
> 
>   /* if this is a cpuset hierarchy, we have to set cgroup.clone_children 
> in
>* the base cgroup, otherwise containers will start with an empty 
> cpuset.mems
>* and cpuset.cpus and then
>*/
> - if (lxc_string_in_array("cpuset", (const char 
> **)mp->hierarchy->subsystems)) {
> - char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
> "/cgroup.clone_children");
> +cpuset:
> + if (lxc_string_in_array("cpuset", subsystems)) {
> + char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
> + "/cgroup.clone_children");
>   if (!cc_path)
> - return -1;
> + goto err;
> +
>   r = lxc_read_from_file(cc_path, buf, 1);
>   if (r == 1 && buf[0] == '1') {
>   free(cc_path);
> - return 0;
> + goto out;
>   }
> +
>   r = lxc_write_to_file(cc_path, "1", 1, false);
> - saved_errno = errno;
> + if (r < 0) {
> + SYSERROR("Failed to set clone_children to 1 for cpuset 
> hierarchy in parent cgroup.");
> + saved_errno = errno;
> + free(cc_path);
> + errno = saved_errno;
> + goto err;
> + }
>   free(cc_path);
> - errno = saved_errno;
> - return r < 0 ? -1 : 0;
>   }
> +out:
>   return 0;
> +err:
> + return -1;
>  }
> 
>  extern void lxc_monitor_send_state(const char *name, lxc_state_t state,
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH 2/3] lxc-start: store PID file to lxc_container

2014-01-21 Thread Qiang Huang 
So we can remove PID file untill lxc_container_free.

This also fix bug: https://github.com/lxc/lxc/issues/89

Signed-off-by: Qiang Huang 
---
 src/lxc/lxc_start.c| 9 +
 src/lxc/lxccontainer.c | 7 +++
 src/lxc/lxccontainer.h | 6 ++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
index d5379da..fd2dc6e 100644
--- a/src/lxc/lxc_start.c
+++ b/src/lxc/lxc_start.c
@@ -302,6 +302,11 @@ int main(int argc, char *argv[])
}
 
if (my_args.pidfile != NULL) {
+   if (ensure_path(&c->pidfile, my_args.pidfile) < 0) {
+   ERROR("failed to ensure pidfile '%s'", my_args.pidfile);
+   goto out;
+   }
+
pid_fp = fopen(my_args.pidfile, "w");
if (pid_fp == NULL) {
SYSERROR("failed to create pidfile '%s' for '%s'",
@@ -342,10 +347,6 @@ int main(int argc, char *argv[])
c->want_close_all_fds(c, true);
 
err = c->start(c, 0, args) ? 0 : -1;
-
-   if (my_args.pidfile)
-   unlink(my_args.pidfile);
-
 out:
lxc_container_put(c);
if (pid_fp)
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index d020918..579c50c 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -237,6 +237,12 @@ static void lxc_container_free(struct lxc_container *c)
free(c->config_path);
c->config_path = NULL;
}
+   if (c->pidfile) {
+   unlink(c->pidfile);
+   free(c->pidfile);
+   c->pidfile = NULL;
+   }
+
free(c);
 }
 
@@ -3121,6 +3127,7 @@ struct lxc_container *lxc_container_new(const char *name, 
const char *configpath
lxcapi_clear_config(c);
}
c->daemonize = true;
+   c->pidfile = NULL;
 
// assign the member functions
c->is_defined = lxcapi_is_defined;
diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
index f3c7d72..84caa31 100644
--- a/src/lxc/lxccontainer.h
+++ b/src/lxc/lxccontainer.h
@@ -68,6 +68,12 @@ struct lxc_container {
 
/*!
 * \private
+* File to store pid.
+*/
+   char *pidfile;
+
+   /*!
+* \private
 * Container semaphore lock.
 */
struct lxc_lock *slock;
-- 
1.8.3


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH 1/3] lxc-start: fix the container leak when daemonize

2014-01-21 Thread Qiang Huang 
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 
Signed-off-by: Qiang Huang 
---
 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);
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

[lxc-devel] [PATCH 3/3] daemon: fix the wrong pid in daemon model

2014-01-21 Thread Qiang Huang 
When you start a container in daemon model, you have at least
3 processes:
 1. The command the user start (lxc-start -d)
 2. The backgrounded fork of that command after start() is done
 3. The container init process

In PID file, we need (2), but currently we are writing (1),
this is wrong because (1) exits as soon as the container is
started, it's complately useless.

So we write pid after daemonize, so that we'll always write
the right pid to PID file.

Reported-by: St��phane Graber 
Signed-off-by: Qiang Huang 
---
 src/lxc/lxc_start.c| 19 ---
 src/lxc/lxccontainer.c | 30 +-
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
index fd2dc6e..4b6b1f7 100644
--- a/src/lxc/lxc_start.c
+++ b/src/lxc/lxc_start.c
@@ -211,7 +211,6 @@ int main(int argc, char *argv[])
"/sbin/init",
'\0',
};
-   FILE *pid_fp = NULL;
struct lxc_container *c;
 
lxc_list_init(&defines);
@@ -306,13 +305,6 @@ int main(int argc, char *argv[])
ERROR("failed to ensure pidfile '%s'", my_args.pidfile);
goto out;
}
-
-   pid_fp = fopen(my_args.pidfile, "w");
-   if (pid_fp == NULL) {
-   SYSERROR("failed to create pidfile '%s' for '%s'",
-my_args.pidfile, my_args.name);
-   goto out;
-   }
}
 
int i;
@@ -334,23 +326,12 @@ int main(int argc, char *argv[])
c->want_daemonize(c, false);
}
 
-   if (pid_fp != NULL) {
-   if (fprintf(pid_fp, "%d\n", getpid()) < 0) {
-   SYSERROR("failed to write '%s'", my_args.pidfile);
-   goto out;
-   }
-   fclose(pid_fp);
-   pid_fp = NULL;
-   }
-
if (my_args.close_all_fds)
c->want_close_all_fds(c, true);
 
err = c->start(c, 0, args) ? 0 : -1;
 out:
lxc_container_put(c);
-   if (pid_fp)
-   fclose(pid_fp);
return err;
 }
 
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 579c50c..c949526 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -545,6 +545,7 @@ static bool lxcapi_start(struct lxc_container *c, int 
useinit, char * const argv
int ret;
struct lxc_conf *conf;
bool daemonize = false;
+   FILE *pid_fp = NULL;
char *default_args[] = {
"/sbin/init",
'\0',
@@ -600,8 +601,13 @@ static bool lxcapi_start(struct lxc_container *c, int 
useinit, char * const argv
pid_t pid = fork();
if (pid < 0)
return false;
-   if (pid != 0)
+   if (pid != 0) {
+   /* Set to NULL because we don't want father unlink
+* the PID file, child will do the free and unlink.
+*/
+   c->pidfile = NULL;
return wait_on_daemonized_start(c, pid);
+   }
 
/* second fork to be reparented by init */
pid = fork();
@@ -630,6 +636,28 @@ static bool lxcapi_start(struct lxc_container *c, int 
useinit, char * const argv
}
}
 
+   /* We need to write PID file after daeminize, so we alway
+* write the right PID.
+*/
+   if (c->pidfile) {
+   pid_fp = fopen(c->pidfile, "w");
+   if (pid_fp == NULL) {
+   SYSERROR("Failed to create pidfile '%s' for '%s'",
+c->pidfile, c->name);
+   return false;
+   }
+
+   if (fprintf(pid_fp, "%d\n", getpid()) < 0) {
+   SYSERROR("Failed to write '%s'", c->pidfile);
+   fclose(pid_fp);
+   pid_fp = NULL;
+   return false;
+   }
+   
+   fclose(pid_fp);
+   pid_fp = NULL;
+   }
+
 reboot:
conf->reboot = 0;
ret = lxc_start(c->name, argv, conf, c->config_path);
-- 
1.8.3


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped

2014-01-20 Thread Qiang Huang 
On 2014/1/21 11:38, Stéphane Graber wrote:
> On Tue, Jan 21, 2014 at 11:33:42AM +0800, Qiang Huang wrote:
>> On 2014/1/21 11:20, Stéphane Graber wrote:
>>> On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote:
>>>> Hi Stéphane,
>>>>
>>>> I'm not sure I understand this.
>>>>
>>>> In my understanding, anything using the API to control an already
>>>> running backgrounded container will use lxc_container_new to create
>>>> a new lxc_container, it only share the original one's name and config
>>>> and so on. After dealing, will call lxc_container_put to free this
>>>> lxc_container, this *new* lxc_container doesn't contain the PID file
>>>> information, so it's freeing won't remove PID file.
>>>
>>> Hmm, yeah, seems like you're right, I should have rechecked the patch
>>> rather than speak from the vague memory of what I saw when you initialy
>>> posted it.
>>>
>>>
>>> So, re-reading more carefully, I don't completely disagree with the
>>> approach, though I don't like having to extend lxc_container just for
>>> that...
>>
>> Without 1/2 patch, this patch won't work, I'll send new patches.
>> So would it be better if I put this into lxc_conf?
>>
>>>
>>> There's however one rather big problem, the PID file will contain the
>>> wrong PID. The PID being written is getpid() from the parent lxc-start,
>>> not the running, forked lxc-start process and that child PID is only
>>> known by the start() function in lxccontainer.c, so you can't create the
>>> PID file from lxc_start.c
>>
>> Yeah, I noticed that before, I thought that's exactly what we want,
>> because we can get the init pid of container from lxc_info command.
>>
>> If it's not, we can fix it incrementally. :)
> 
> No, what I meant is that the PID we are writing is that of a
> non-existing process, which makes the pid file entirely useless.
> 
> When you start a container, you have at least 3 processes:
>  1) The command the user start (lxc-start -d)
>  2) The backgrounded fork of that command after start() is done
>  3) The container init process
> 
> lxc-info and the API init_pid() function gives us (3), what we want to
> see in a pidfile is (2) and what we are currently writing is (1).
> 
> As (1) exits as soon as the container is started, it's completely
> useless as you can't use that to check whether LXC still has control
> over the container (i.e. is lxc-start still running in the background).
> 

Oh, that's right, in the daemon model the PID file stores wrong pid,
I'll think about it and fix it in the new patch.

Thanks Stéphane.


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 1/2] lxc-start: fix the container leak when daemonize

2014-01-20 Thread Qiang Huang 
On 2014/1/21 11:22, Serge Hallyn wrote:
> Quoting Qiang Huang (h.huangqi...@huawei.com):
>> Hi Serge,
>>
>> On 2014/1/20 23:44, Serge Hallyn wrote:
>>> Quoting Qiang Huang (h.huangqi...@huawei.com):
>>>> On 2014/1/20 1:26, 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. Even his father return to main(), the
>>>>>> lxc_container_put won't affect son's numthreads.
>>>>>
>>>>> The memlock is only between threads.  But the child is fork()ed, so the
>>>>> lxc_container_put() of one won't affect the other task.
>>>>>
>>>>> Now maybe wait_on_daemonized_start() should be doing an
>>>>> lxc_container_put()?
>>>>>
>>>>>> So when daemon stops, he should return to main and do
>>>>>> lxc_container_put again, rather than exit and leave the
>>>>>> container alone.
>>>>>
>>>>> I disagree.  This means two separate processes will continue at the
>>>>> point where lxcapi_start() was called.  That sounds like a recipe for
>>>>> disaster.
>>>>
>>>> Well, I thought as long as child haven't exec(), he'll still have
>>>> the same context as his father, where lxcapi_start() was called
>>>> is part of it. So I don't see how disaster that will be.
>>>
>>> The disaster would be that the caller might do something like:
>>>
>>> c->want_daemonize(true);
>>> if (c->start()) {
>>> FILE *f = fopen("/run/running_containers", "a");
>>> bump_container_count(f);
>>> fclose(f);
>>> }
>>>
>>> Now the caller's thread will bump the running_containers count,
>>> and then after the container shuts down, the (daemonized)
>>> monitor thread (which never does an exec) will return and also
>>> bump that count, making the count in /run/running_containers
>>> wrong.
>>
>> The real world daemonized monitor thread dose an exec ;)
> 
> When lxcapi_start() forks, the new task in turn will also clone,
> so that there is both a container monitor task and the actual
> init task.  The init task is the one that execs.  The monitor
> task does not do an exec.  That is the one which must exit when
> it is done.

Yeah, I understand this, I think we were talking about different
monitors, the one I said is from lxc_monitord_spawn, it exec()
to run lxc-monitord.

Anyway, I think we have got consensus view, I'll send new patches
with that PID file one.

Thanks again.

> 
> -serge
> 
> .
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped

2014-01-20 Thread Qiang Huang 
On 2014/1/21 11:20, Stéphane Graber wrote:
> On Tue, Jan 21, 2014 at 10:54:50AM +0800, Qiang Huang wrote:
>> Hi Stéphane,
>>
>> I'm not sure I understand this.
>>
>> In my understanding, anything using the API to control an already
>> running backgrounded container will use lxc_container_new to create
>> a new lxc_container, it only share the original one's name and config
>> and so on. After dealing, will call lxc_container_put to free this
>> lxc_container, this *new* lxc_container doesn't contain the PID file
>> information, so it's freeing won't remove PID file.
> 
> Hmm, yeah, seems like you're right, I should have rechecked the patch
> rather than speak from the vague memory of what I saw when you initialy
> posted it.
> 
> 
> So, re-reading more carefully, I don't completely disagree with the
> approach, though I don't like having to extend lxc_container just for
> that...

Without 1/2 patch, this patch won't work, I'll send new patches.
So would it be better if I put this into lxc_conf?

> 
> There's however one rather big problem, the PID file will contain the
> wrong PID. The PID being written is getpid() from the parent lxc-start,
> not the running, forked lxc-start process and that child PID is only
> known by the start() function in lxccontainer.c, so you can't create the
> PID file from lxc_start.c

Yeah, I noticed that before, I thought that's exactly what we want,
because we can get the init pid of container from lxc_info command.

If it's not, we can fix it incrementally. :)

> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 1/2] lxc-start: fix the container leak when daemonize

2014-01-20 Thread Qiang Huang 
Hi Serge,

On 2014/1/20 23:44, Serge Hallyn wrote:
> Quoting Qiang Huang (h.huangqi...@huawei.com):
>> On 2014/1/20 1:26, 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. Even his father return to main(), the
>>>> lxc_container_put won't affect son's numthreads.
>>>
>>> The memlock is only between threads.  But the child is fork()ed, so the
>>> lxc_container_put() of one won't affect the other task.
>>>
>>> Now maybe wait_on_daemonized_start() should be doing an
>>> lxc_container_put()?
>>>
>>>> So when daemon stops, he should return to main and do
>>>> lxc_container_put again, rather than exit and leave the
>>>> container alone.
>>>
>>> I disagree.  This means two separate processes will continue at the
>>> point where lxcapi_start() was called.  That sounds like a recipe for
>>> disaster.
>>
>> Well, I thought as long as child haven't exec(), he'll still have
>> the same context as his father, where lxcapi_start() was called
>> is part of it. So I don't see how disaster that will be.
> 
> The disaster would be that the caller might do something like:
> 
>   c->want_daemonize(true);
>   if (c->start()) {
>   FILE *f = fopen("/run/running_containers", "a");
>   bump_container_count(f);
>   fclose(f);
>   }
> 
> Now the caller's thread will bump the running_containers count,
> and then after the container shuts down, the (daemonized)
> monitor thread (which never does an exec) will return and also
> bump that count, making the count in /run/running_containers
> wrong.

The real world daemonized monitor thread dose an exec ;)

But yes, I agree we can do some bad things based on this scenario,
a new process should exit rather than return to where his
birth function was called.

Thanks Serge.

> 
> -serge
> 
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped

2014-01-20 Thread Qiang Huang 
Hi Stéphane,

On 2014/1/21 0:07, Stéphane Graber wrote:
> On Mon, Jan 20, 2014 at 03:22:31PM +0800, Qiang Huang wrote:
>> On 2014/1/19 8:57, Stéphane Graber wrote:
>>> On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
>>>> This is for bug: https://github.com/lxc/lxc/issues/89
>>>>
>>>> When start container with daemon model, the daemon process's
>>>> father will return back to main in start time, and pidfile
>>>> is removed then, that's not right.
>>>> So we store pidfile to lxc_container, and remove it when
>>>> lxc_container_free.
>>>
>>> That one looks wrong to me, removing the pid file on lxc_container_free
>>> is wrong. We want the pidfile removed when the background lxc-start
>>> exits, not whenever a random API client flushes the container from
>>> memory.
>>>
>>> With your patch, doing something like:
>>>  - lxc-start -n p1 -d -p /tmp/pid
>>>  - python3
>>>  import lxc
>>>  p1 = lxc.Container("p1")
>>>  p1 = None
>>
>> I'm sorry I'm not family with python, can you explain how this would
>> happen in real world? Thanks.
> 
> In the real world, anything using the API to control an already running
> backgrounded container with a PID file and that does lxc_container_put
> once it's done dealing with the container object will cause the PID file
> to be removed.

I'm not sure I understand this.

In my understanding, anything using the API to control an already
running backgrounded container will use lxc_container_new to create
a new lxc_container, it only share the original one's name and config
and so on. After dealing, will call lxc_container_put to free this
lxc_container, this *new* lxc_container doesn't contain the PID file
information, so it's freeing won't remove PID file.

Anyway, my patch is based on one theory:
A running container will have an lxc_container to hold it's information,
which is created from start, the struct lxc_container lasts for all of
container's life cycle.

If it's wrong, my question is:
How could it be reasonable for a container running without an lxc_container?

> 
> Actually, even calling lxc-list should cause the PID file to be removed
> as lxc-list calls list_containers which calls list_defined_containers
> which in turns iterate through all containers, get their struct and then
> lxc_container_put them.
> 
>>
>>>
>>> Or the equivalent with any binding, or directly in C, will destroy the
>>> pid file even though the container is clearly still running.
>>
>> I thought when the backgroud lxc-start exits, it's time for container
>> to free, because there are no other place to do lxc_contaier_get to
>> hold the container from freeing.
>>
>> I must missed something :( , so waiting for your more details.
>>
>>
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] cgroup.c: clean up for handle_cgroup_settings

2014-01-20 Thread Qiang Huang 
Clean up the nesting if, make the logic similar for memory
and cpuset, and the error message should sent from inside,
for better extendibility.

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.c | 53 ++---
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 85384fc..693db94 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -782,10 +782,8 @@ struct cgroup_process_info *lxc_cgroupfs_create(const char 
*name, const char *pa

if (lxc_string_in_array("ns", (const char **)h->subsystems))
continue;
-   if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0) {
-   ERROR("Could not set clone_children to 1 for cpuset 
hierarchy in parent cgroup.");
+   if (handle_cgroup_settings(mp, info_ptr->cgroup_path) < 0)
goto out_initial_error;
-   }
}

/* normalize the path */
@@ -2016,43 +2014,60 @@ static int handle_cgroup_settings(struct 
cgroup_mount_point *mp,
 {
int r, saved_errno = 0;
char buf[2];
+   const char **subsystems = (const char **)mp->hierarchy->subsystems;

/* If this is the memory cgroup, we want to enforce hierarchy.
 * But don't fail if for some reason we can't.
 */
-   if (lxc_string_in_array("memory", (const char 
**)mp->hierarchy->subsystems)) {
-   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
"/memory.use_hierarchy");
-   if (cc_path) {
-   r = lxc_read_from_file(cc_path, buf, 1);
-   if (r < 1 || buf[0] != '1') {
-   r = lxc_write_to_file(cc_path, "1", 1, false);
-   if (r < 0)
-   SYSERROR("failed to set 
memory.use_hiararchy to 1; continuing");
-   }
+   if (lxc_string_in_array("memory", subsystems)) {
+   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
+   "/memory.use_hierarchy");
+   if (!cc_path)
+   goto cpuset;
+
+   r = lxc_read_from_file(cc_path, buf, 1);
+   if (r == 1 && buf[0] == '1') {
free(cc_path);
+   goto cpuset;
}
+
+   r = lxc_write_to_file(cc_path, "1", 1, false);
+   if (r < 0)
+   SYSERROR("Failed to set memory.use_hiararchy to 1; 
continuing");
+   free(cc_path);
}

/* if this is a cpuset hierarchy, we have to set cgroup.clone_children 
in
 * the base cgroup, otherwise containers will start with an empty 
cpuset.mems
 * and cpuset.cpus and then
 */
-   if (lxc_string_in_array("cpuset", (const char 
**)mp->hierarchy->subsystems)) {
-   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path, 
"/cgroup.clone_children");
+cpuset:
+   if (lxc_string_in_array("cpuset", subsystems)) {
+   char *cc_path = cgroup_to_absolute_path(mp, cgroup_path,
+   "/cgroup.clone_children");
if (!cc_path)
-   return -1;
+   goto err;
+
r = lxc_read_from_file(cc_path, buf, 1);
if (r == 1 && buf[0] == '1') {
free(cc_path);
-   return 0;
+   goto out;
}
+
r = lxc_write_to_file(cc_path, "1", 1, false);
-   saved_errno = errno;
+   if (r < 0) {
+   SYSERROR("Failed to set clone_children to 1 for cpuset 
hierarchy in parent cgroup.");
+   saved_errno = errno;
+   free(cc_path);
+   errno = saved_errno;
+   goto err;
+   }
free(cc_path);
-   errno = saved_errno;
-   return r < 0 ? -1 : 0;
}
+out:
return 0;
+err:
+   return -1;
 }

 extern void lxc_monitor_send_state(const char *name, lxc_state_t state,
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped

2014-01-19 Thread Qiang Huang 
On 2014/1/19 8:57, Stéphane Graber wrote:
> On Sat, Jan 18, 2014 at 03:00:00PM +0800, Qiang Huang wrote:
>> This is for bug: https://github.com/lxc/lxc/issues/89
>>
>> When start container with daemon model, the daemon process's
>> father will return back to main in start time, and pidfile
>> is removed then, that's not right.
>> So we store pidfile to lxc_container, and remove it when
>> lxc_container_free.
> 
> That one looks wrong to me, removing the pid file on lxc_container_free
> is wrong. We want the pidfile removed when the background lxc-start
> exits, not whenever a random API client flushes the container from
> memory.
> 
> With your patch, doing something like:
>  - lxc-start -n p1 -d -p /tmp/pid
>  - python3
>  import lxc
>  p1 = lxc.Container("p1")
>  p1 = None

I'm sorry I'm not family with python, can you explain how this would
happen in real world? Thanks.

> 
> Or the equivalent with any binding, or directly in C, will destroy the
> pid file even though the container is clearly still running.

I thought when the backgroud lxc-start exits, it's time for container
to free, because there are no other place to do lxc_contaier_get to
hold the container from freeing.

I must missed something :( , so waiting for your more details.


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] about LXC's coding style

2014-01-19 Thread Qiang Huang 
On 2014/1/20 1:54, Serge Hallyn wrote:
> Quoting Qiang Huang (h.huangqi...@huawei.com):
>> Hi Serge and Stéphane and list,
>>
>> I'm sorry but I need to complain about this :(
>>
>> I saw LXC's CONTRIBUTING file, it says:
>> "The coding style follows the Linux kernel coding style"
>>
>> But after reading code these days, there are too many very-long-line
>> codes, especially in cgroup.c, this really cause some inconvenience,
>> when reading LXC code, I can't vsplit my vim any more :(
> 
> I think that's overstating it...  I do it all the time and I have
> tiny cheap low-resolution laptops.
> 
>> I don't know if this is an issue for other guys, can we keep the
>> rules for the future reviews?
>>
>> Thanks and best regards.
> 
> It's something I've had mixed feelings about over the years.  I'd
> like to enforce it, but the code never really followed that to
> begin with and I haven't wanted to have some big patch doing random
> code churn to follow that guideline.
> 
> The vsplitting is a minor nuisance, and that effect is mentioned
> in the guidelines, but the *main* point of that rule is not for
> controlling line length itself.  The main point is to de-facto
> control condition nesting in functions.  So if you have too much
> 
>   if (a)
>   if (b)
>   if (c)
>   ..

Yes, vspliting is a minor complain, the bad nesting and bad naming
and bad format are my real points.

I'll try to clean up some of that conditions when I spotted again.

> 
> then you should be considering introducing some well-named helper
> functions.  I do try to enforce that rule, and patches (small,
> targeted, clear) to improve code flow would be welcome.
> 
> -serge
> 
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH RFC] api_start: don't get a container reference for the daemonized case

2014-01-19 Thread Qiang Huang 
On 2014/1/20 2:17, Serge Hallyn wrote:
> In the daemonized case we will fork, so the anonymous container memlock
> will not be shared between parent and child.

I'm basically agree with this patch.

And I'm also confused about why we need this lxc_container_get
in the first place. Seems like it only work for multi-threads cases,
not this fork() case, because any writing lxc_container will cause
COW, and father's lxc_container_free won't affect child.

After a bit dig, I found this is the only place where uses
lxc_container_get(there are two more in test code, not necessary), so
maybe we can just remove this api till we really need it?

Or maybe I'm missing something, hope somebody clear me up. :)

> 
> Please review.
> 
> Signed-off-by: Serge Hallyn 
> ---
>  src/lxc/lxccontainer.c | 12 +++-
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 0bebdff..0e22ccf 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -583,15 +583,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);
>  
> @@ -632,12 +628,10 @@ reboot:
>   goto reboot;
>   }
>  
> - if (daemonize) {
> - lxc_container_put(c);

Can we leave this put operation here to free lxc_container? Although this
is not a really memory leak, but for this process, it still have
numthreads = 1 and have no where to minus it.

> + if (daemonize)
>   exit (ret == 0 ? true : false);
> - } else {
> + else
>   return (ret == 0 ? true : false);
> - }
>  }
>  
>  /*
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH 1/2] lxc-start: fix the container leak when daemonize

2014-01-19 Thread Qiang Huang 
On 2014/1/20 1:26, 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. Even his father return to main(), the
>> lxc_container_put won't affect son's numthreads.
> 
> The memlock is only between threads.  But the child is fork()ed, so the
> lxc_container_put() of one won't affect the other task.
> 
> Now maybe wait_on_daemonized_start() should be doing an
> lxc_container_put()?
> 
>> So when daemon stops, he should return to main and do
>> lxc_container_put again, rather than exit and leave the
>> container alone.
> 
> I disagree.  This means two separate processes will continue at the
> point where lxcapi_start() was called.  That sounds like a recipe for
> disaster.

Well, I thought as long as child haven't exec(), he'll still have
the same context as his father, where lxcapi_start() was called
is part of it. So I don't see how disaster that will be.

But yes, I agree with the other patch you sent, maybe better solution,
I'll check that again.

> 
>> Signed-off-by: Qiang Huang 
> 
> I'm basically out this week and may not be thinking about it rightly,
> but I'm going to say
> 
> Nacked-by: Serge E. Hallyn 
> 
> in the hopes that someone with enough time will reconsider.
> 
>> ---
>>  src/lxc/lxccontainer.c | 8 +++-
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
>> index 0bebdff..ddea0d7 100644
>> --- a/src/lxc/lxccontainer.c
>> +++ b/src/lxc/lxccontainer.c
>> @@ -632,12 +632,10 @@ reboot:
>>  goto reboot;
>>  }
>>
>> -if (daemonize) {
>> +if (daemonize)
>>  lxc_container_put(c);
>> -exit (ret == 0 ? true : false);
>> -} else {
>> -return (ret == 0 ? true : false);
>> -}
>> +
>> +return (ret == 0 ? true : false);
>>  }
>>
>>  /*
>> -- 
>> 1.8.3
>>
> 
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] about LXC's coding style

2014-01-17 Thread Qiang Huang 
Hi Serge and Stéphane and list,

I'm sorry but I need to complain about this :(

I saw LXC's CONTRIBUTING file, it says:
"The coding style follows the Linux kernel coding style"

But after reading code these days, there are too many very-long-line
codes, especially in cgroup.c, this really cause some inconvenience,
when reading LXC code, I can't vsplit my vim any more :(

I don't know if this is an issue for other guys, can we keep the
rules for the future reviews?

Thanks and best regards.

Qiang

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH 1/2] lxc-start: fix the container leak when daemonize

2014-01-17 Thread Qiang Huang 
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. Even his father return to main(), the
lxc_container_put won't affect son's numthreads.

So when daemon stops, he should return to main and do
lxc_container_put again, rather than exit and leave the
container alone.

Signed-off-by: Qiang Huang 
---
 src/lxc/lxccontainer.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 0bebdff..ddea0d7 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -632,12 +632,10 @@ reboot:
goto reboot;
}

-   if (daemonize) {
+   if (daemonize)
lxc_container_put(c);
-   exit (ret == 0 ? true : false);
-   } else {
-   return (ret == 0 ? true : false);
-   }
+   
+   return (ret == 0 ? true : false);
 }

 /*
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH 2/2] daemon: remove pidfile when daemon container is stopped

2014-01-17 Thread Qiang Huang 
This is for bug: https://github.com/lxc/lxc/issues/89

When start container with daemon model, the daemon process's
father will return back to main in start time, and pidfile
is removed then, that's not right.
So we store pidfile to lxc_container, and remove it when
lxc_container_free.

Signed-off-by: Qiang Huang 
---
 src/lxc/lxc_start.c| 9 +
 src/lxc/lxccontainer.c | 6 ++
 src/lxc/lxccontainer.h | 6 ++
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
index d5379da..fd2dc6e 100644
--- a/src/lxc/lxc_start.c
+++ b/src/lxc/lxc_start.c
@@ -302,6 +302,11 @@ int main(int argc, char *argv[])
}

if (my_args.pidfile != NULL) {
+   if (ensure_path(&c->pidfile, my_args.pidfile) < 0) {
+   ERROR("failed to ensure pidfile '%s'", my_args.pidfile);
+   goto out;
+   }
+
pid_fp = fopen(my_args.pidfile, "w");
if (pid_fp == NULL) {
SYSERROR("failed to create pidfile '%s' for '%s'",
@@ -342,10 +347,6 @@ int main(int argc, char *argv[])
c->want_close_all_fds(c, true);

err = c->start(c, 0, args) ? 0 : -1;
-
-   if (my_args.pidfile)
-   unlink(my_args.pidfile);
-
 out:
lxc_container_put(c);
if (pid_fp)
diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index ddea0d7..d742781 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -231,6 +231,11 @@ static void lxc_container_free(struct lxc_container *c)
free(c->config_path);
c->config_path = NULL;
}
+   if (c->pidfile) {
+   unlink(c->pidfile);
+   free(c->pidfile);
+   c->pidfile = NULL;
+   }
free(c);
 }

@@ -3051,6 +3056,7 @@ struct lxc_container *lxc_container_new(const char *name, 
const char *configpath
lxcapi_clear_config(c);
}
c->daemonize = true;
+   c->pidfile = NULL;

// assign the member functions
c->is_defined = lxcapi_is_defined;
diff --git a/src/lxc/lxccontainer.h b/src/lxc/lxccontainer.h
index 921e47d..a860648 100644
--- a/src/lxc/lxccontainer.h
+++ b/src/lxc/lxccontainer.h
@@ -68,6 +68,12 @@ struct lxc_container {

/*!
 * \private
+* File to store pid.
+*/
+   char *pidfile;
+
+   /*!
+* \private
 * Container semaphore lock.
 */
struct lxc_lock *slock;
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] exclude non-existing signals from the loop

2014-01-17 Thread Qiang Huang 
On 2014年01月17日 22:24, Serge Hallyn wrote:
> If we could know on any system which signals to bypass that'd be
> fine, but AFAICS we can't.
> 
> It sounds to me like we should simply ignore failure at sigaction like
> we used to :)  Something like below.  Is that what you meant?
> 
> From 87319b691c8f65c7d61ee01e64707d0b59d11caa Mon Sep 17 00:00:00 2001
> From: Serge Hallyn 
> Date: Fri, 17 Jan 2014 08:23:18 -0600
> Subject: [PATCH 1/1] lxc_init: don't fail on bad signals
> 
> Signed-off-by: Serge Hallyn 
> ---
>  src/lxc/lxc_init.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_init.c b/src/lxc/lxc_init.c
> index a59dd9c..b86edf8 100644
> --- a/src/lxc/lxc_init.c
> +++ b/src/lxc/lxc_init.c
> @@ -159,8 +159,7 @@ int main(int argc, char *argv[])
>   act.sa_flags = 0;
>   act.sa_handler = interrupt_handler;
>   if (sigaction(i, &act, NULL)) {
> - SYSERROR("failed to sigaction");
> - exit(EXIT_FAILURE);
> + INFO ("failed to sigaction (%d)", i);
>   }
>   }
>  
> 

Yeah, I can live with that :)
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] exclude non-existing signals from the loop

2014-01-16 Thread Qiang Huang 
On 2014/1/17 5:38, Serge Hallyn wrote:
> Quoting S.Çağlar Onur (cag...@10ur.org):
>> On Thu, Jan 16, 2014 at 4:24 PM, Serge Hallyn  
>> wrote:
>>> Quoting S.Çağlar Onur (cag...@10ur.org):
 32 and 33 are not defined and causing sigaction to fail. "kill -l" shows 
 following
 on my system

  1) SIGHUP   2) SIGINT   3) SIGQUIT  4) SIGILL   5) SIGTRAP
  6) SIGABRT  7) SIGBUS   8) SIGFPE   9) SIGKILL 10) SIGUSR1
 11) SIGSEGV 12) SIGUSR2 13) SIGPIPE 14) SIGALRM 15) SIGTERM
 16) SIGSTKFLT   17) SIGCHLD 18) SIGCONT 19) SIGSTOP 20) SIGTSTP
 21) SIGTTIN 22) SIGTTOU 23) SIGURG  24) SIGXCPU 25) SIGXFSZ
 26) SIGVTALRM   27) SIGPROF 28) SIGWINCH29) SIGIO   30) SIGPWR
 31) SIGSYS  34) SIGRTMIN35) SIGRTMIN+1  36) SIGRTMIN+2  37) 
 SIGRTMIN+3
 38) SIGRTMIN+4  39) SIGRTMIN+5  40) SIGRTMIN+6  41) SIGRTMIN+7  42) 
 SIGRTMIN+8
 43) SIGRTMIN+9  44) SIGRTMIN+10 45) SIGRTMIN+11 46) SIGRTMIN+12 47) 
 SIGRTMIN+13
 48) SIGRTMIN+14 49) SIGRTMIN+15 50) SIGRTMAX-14 51) SIGRTMAX-13 52) 
 SIGRTMAX-12
 53) SIGRTMAX-11 54) SIGRTMAX-10 55) SIGRTMAX-9  56) SIGRTMAX-8  57) 
 SIGRTMAX-7
 58) SIGRTMAX-6  59) SIGRTMAX-5  60) SIGRTMAX-4  61) SIGRTMAX-3  62) 
 SIGRTMAX-2
 63) SIGRTMAX-1  64) SIGRTMAX

 Signed-off-by: S.Çağlar Onur 
>>>
>>> Odd...  on my system NSIG is 32, so these should never hit (since it is
>>> in a while i>
>> Printing NSIG via ERROR shows that its 64 on my system.
> 
> So a header file is #defining NSIG, which is already defined to 32
> in kernel headers, to _NSIG, which is 64.
> 
> Looking around the current state of kernel headers, i wonder whether
> we should imply use min(SIGRTMIN, NSIG).

My box is x86_64, and I got the same result as S.Çağlar.

People may use signal number bigger than SIGRTMIN, so min(SIGRTMIN, NSIG)
would miss them. Isn't that cause any problems?

Maybe we should just bypass there non-existent signals.

> 
>>> Does 32 show up in /usr/include/asm-generic/signal.h or
>>> /usr/include/`arch`-linux-gnu/asm/signal.h ?
>>
>> [caglar@qp:~/Projects/lxc(master)] find /usr/include/ -name signal.h |
>> xargs grep NSIG
>> /usr/include/asm-generic/signal.h:#define _NSIG 64
>> /usr/include/asm-generic/signal.h:#define _NSIG_BPW __BITS_PER_LONG
>> /usr/include/asm-generic/signal.h:#define _NSIG_WORDS   (_NSIG / _NSIG_BPW)
>> /usr/include/asm-generic/signal.h:#define SIGRTMAX  _NSIG
>> /usr/include/asm-generic/signal.h:#define MINSIGSTKSZ   2048
>> /usr/include/asm-generic/signal.h:  unsigned long sig[_NSIG_WORDS];
>> /usr/include/x86_64-linux-gnu/asm/signal.h:#define NSIG 32
>> /usr/include/x86_64-linux-gnu/asm/signal.h:#define SIGRTMAX _NSIG
>> /usr/include/x86_64-linux-gnu/asm/signal.h:#define MINSIGSTKSZ  2048
>> /usr/include/signal.h:# define NSIG _NSIG
>> /usr/include/signal.h:extern const char *const _sys_siglist[_NSIG];
>> /usr/include/signal.h:extern const char *const sys_siglist[_NSIG];
>>
 ---
  src/lxc/lxc_init.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/src/lxc/lxc_init.c b/src/lxc/lxc_init.c
 index a59dd9c..ae64af8 100644
 --- a/src/lxc/lxc_init.c
 +++ b/src/lxc/lxc_init.c
 @@ -143,7 +143,8 @@ int main(int argc, char *argv[])
   i == SIGSEGV ||
   i == SIGBUS ||
   i == SIGSTOP ||
 - i == SIGKILL)
 + i == SIGKILL ||
 + i == 32 || i == 33)
   continue;

   if (sigfillset(&act.sa_mask) ||
 --
 1.8.3.2



___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] Dropping lxc-checkpoint and lxc-restart

2014-01-16 Thread Qiang Huang 
On 2014/1/17 9:19, Stéphane Graber wrote:
> I just noticed that we still have lxc-checkpoint and lxc-restart in the
> upstream branch, as far as I know those two only work with patched
> kernel using a patchset which hasn't been updated in ages.
> 
> I'd recommend we get those two tools out of our codebase for the time
> being and only bring them back to life once we have CRIU support in
> liblxc and can base their replacements on that API.
> 
> Any objections?

No objections at all.
I'd like LXC 1.0 to be right and clean :)

> 
> 
> 
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] lxc_init.c: error handing for sigaction and sigprocmask

2014-01-15 Thread Qiang Huang 
Look through all LXC code and seems like only here are missed.

Signed-off-by: Qiang Huang 
---
Maybe this bug can be marked resolved:
https://github.com/lxc/lxc/issues/83
---
 src/lxc/lxc_init.c | 46 +++---
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/src/lxc/lxc_init.c b/src/lxc/lxc_init.c
index d88a935..a59dd9c 100644
--- a/src/lxc/lxc_init.c
+++ b/src/lxc/lxc_init.c
@@ -123,11 +123,14 @@ int main(int argc, char *argv[])
 * mask all the signals so we are safe to install a
 * signal handler and to fork
 */
-   sigfillset(&mask);
-   sigdelset(&mask, SIGILL);
-   sigdelset(&mask, SIGSEGV);
-   sigdelset(&mask, SIGBUS);
-   sigprocmask(SIG_SETMASK, &mask, &omask);
+   if (sigfillset(&mask) ||
+   sigdelset(&mask, SIGILL) ||
+   sigdelset(&mask, SIGSEGV) ||
+   sigdelset(&mask, SIGBUS) ||
+   sigprocmask(SIG_SETMASK, &mask, &omask)) {
+   SYSERROR("failed to set signal mask");
+   exit(EXIT_FAILURE);
+   }

for (i = 1; i < NSIG; i++) {
struct sigaction act;
@@ -143,15 +146,22 @@ int main(int argc, char *argv[])
i == SIGKILL)
continue;

-   sigfillset(&act.sa_mask);
-   sigdelset(&act.sa_mask, SIGILL);
-   sigdelset(&act.sa_mask, SIGSEGV);
-   sigdelset(&act.sa_mask, SIGBUS);
-   sigdelset(&act.sa_mask, SIGSTOP);
-   sigdelset(&act.sa_mask, SIGKILL);
+   if (sigfillset(&act.sa_mask) ||
+   sigdelset(&act.sa_mask, SIGILL) ||
+   sigdelset(&act.sa_mask, SIGSEGV) ||
+   sigdelset(&act.sa_mask, SIGBUS) ||
+   sigdelset(&act.sa_mask, SIGSTOP) ||
+   sigdelset(&act.sa_mask, SIGKILL)) {
+   ERROR("failed to set signal");
+   exit(EXIT_FAILURE);
+   }
+
act.sa_flags = 0;
act.sa_handler = interrupt_handler;
-   sigaction(i, &act, NULL);
+   if (sigaction(i, &act, NULL)) {
+   SYSERROR("failed to sigaction");
+   exit(EXIT_FAILURE);
+   }
}

lxc_setup_fs();
@@ -170,7 +180,10 @@ int main(int argc, char *argv[])
for (i = 1; i < NSIG; i++)
signal(i, SIG_DFL);

-   sigprocmask(SIG_SETMASK, &omask, NULL);
+   if (sigprocmask(SIG_SETMASK, &omask, NULL)) {
+   SYSERROR("failed to set signal mask");
+   exit(EXIT_FAILURE);
+   }

NOTICE("about to exec '%s'", aargv[0]);

@@ -180,8 +193,11 @@ int main(int argc, char *argv[])
}

/* let's process the signals now */
-   sigdelset(&omask, SIGALRM);
-   sigprocmask(SIG_SETMASK, &omask, NULL);
+   if (sigdelset(&omask, SIGALRM) ||
+   sigprocmask(SIG_SETMASK, &omask, NULL)) {
+   SYSERROR("failed to set signal mask");
+   exit(EXIT_FAILURE);
+   }

/* no need of other inherited fds but stderr */
close(fileno(stdin));
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH 1/2] cgroup.c: add static keywords as they declared

2014-01-15 Thread Qiang Huang 
From: Qiang Huang 

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.c | 47 +--
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 6d837f9..8030a8b 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -63,7 +63,6 @@ static char **subsystems_from_mount_options(const char 
*mount_options, char **ke
 static void lxc_cgroup_mount_point_free(struct cgroup_mount_point *mp);
 static void lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h);
 static bool is_valid_cgroup(const char *name);
-static int create_or_remove_cgroup(bool remove, struct cgroup_mount_point *mp, 
const char *path, int recurse);
 static int create_cgroup(struct cgroup_mount_point *mp, const char *path);
 static int remove_cgroup(struct cgroup_mount_point *mp, const char *path, bool 
recurse);
 static char *cgroup_to_absolute_path(struct cgroup_mount_point *mp, const char 
*path, const char *suffix);
@@ -1515,7 +1514,9 @@ int lxc_cgroup_nrtasks_handler(struct lxc_handler 
*handler)
return ret;
 }

-struct cgroup_process_info *lxc_cgroup_process_info_getx(const char 
*proc_pid_cgroup_str, struct cgroup_meta_data *meta)
+static struct cgroup_process_info *
+lxc_cgroup_process_info_getx(const char *proc_pid_cgroup_str,
+struct cgroup_meta_data *meta)
 {
struct cgroup_process_info *result = NULL;
FILE *proc_pid_cgroup = NULL;
@@ -1610,7 +1611,8 @@ out_error:
return NULL;
 }

-char **subsystems_from_mount_options(const char *mount_options, char 
**kernel_list)
+static char **subsystems_from_mount_options(const char *mount_options,
+   char **kernel_list)
 {
char *token, *str, *saveptr = NULL;
char **result = NULL;
@@ -1647,7 +1649,7 @@ out_free:
return NULL;
 }

-void lxc_cgroup_mount_point_free(struct cgroup_mount_point *mp)
+static void lxc_cgroup_mount_point_free(struct cgroup_mount_point *mp)
 {
if (!mp)
return;
@@ -1656,7 +1658,7 @@ void lxc_cgroup_mount_point_free(struct 
cgroup_mount_point *mp)
free(mp);
 }

-void lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h)
+static void lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h)
 {
if (!h)
return;
@@ -1665,7 +1667,7 @@ void lxc_cgroup_hierarchy_free(struct cgroup_hierarchy *h)
free(h);
 }

-bool is_valid_cgroup(const char *name)
+static bool is_valid_cgroup(const char *name)
 {
const char *p;
for (p = name; *p; p++) {
@@ -1675,7 +1677,8 @@ bool is_valid_cgroup(const char *name)
return strcmp(name, ".") != 0 && strcmp(name, "..") != 0;
 }

-int create_or_remove_cgroup(bool do_remove, struct cgroup_mount_point *mp, 
const char *path, int recurse)
+static int create_or_remove_cgroup(bool do_remove,
+   struct cgroup_mount_point *mp, const char *path, int recurse)
 {
int r, saved_errno = 0;
char *buf = cgroup_to_absolute_path(mp, path, NULL);
@@ -1696,17 +1699,19 @@ int create_or_remove_cgroup(bool do_remove, struct 
cgroup_mount_point *mp, const
return r;
 }

-int create_cgroup(struct cgroup_mount_point *mp, const char *path)
+static int create_cgroup(struct cgroup_mount_point *mp, const char *path)
 {
return create_or_remove_cgroup(false, mp, path, false);
 }

-int remove_cgroup(struct cgroup_mount_point *mp, const char *path, bool 
recurse)
+static int remove_cgroup(struct cgroup_mount_point *mp,
+const char *path, bool recurse)
 {
return create_or_remove_cgroup(true, mp, path, recurse);
 }

-char *cgroup_to_absolute_path(struct cgroup_mount_point *mp, const char *path, 
const char *suffix)
+static char *cgroup_to_absolute_path(struct cgroup_mount_point *mp,
+const char *path, const char *suffix)
 {
/* first we have to make sure we subtract the mount point's prefix */
char *prefix = mp->mount_prefix;
@@ -1750,7 +1755,8 @@ char *cgroup_to_absolute_path(struct cgroup_mount_point 
*mp, const char *path, c
return buf;
 }

-struct cgroup_process_info *find_info_for_subsystem(struct cgroup_process_info 
*info, const char *subsystem)
+static struct cgroup_process_info *
+find_info_for_subsystem(struct cgroup_process_info *info, const char 
*subsystem)
 {
struct cgroup_process_info *info_ptr;
for (info_ptr = info; info_ptr; info_ptr = info_ptr->next) {
@@ -1762,7 +1768,8 @@ struct cgroup_process_info 
*find_info_for_subsystem(struct cgroup_process_info *
return NULL;
 }

-int do_cgroup_get(const char *cgroup_path, const char *sub_filename, char 
*value, size_t len)
+static int do_cgroup_get(const char *cgroup_path, const char *sub_filename,
+char *value, size_t len)
 {
const char *parts[3] = {
cgroup_path,
@@ -1783,7 +1790,8 @@ int do_cgroup_

[lxc-devel] [PATCH 2/2] cgroup.h: unify the nameing and comments

2014-01-15 Thread Qiang Huang 
From: Qiang Huang 

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lxc/cgroup.h b/src/lxc/cgroup.h
index 3aab12d..a252123 100644
--- a/src/lxc/cgroup.h
+++ b/src/lxc/cgroup.h
@@ -155,7 +155,7 @@ extern int lxc_cgroup_get(const char *filename, char 
*value, size_t len, const c
  * Returns path on success, NULL on error. The caller must free()
  * the returned path.
  */
-extern char *lxc_cgroup_path_get(const char *subsystem, const char *name,
+extern char *lxc_cgroup_path_get(const char *filename, const char *name,
  const char *lxcpath);

 struct lxc_list;
-- 
1.8.3.2
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] cgroup.c: redefine the valid cgroup name

2014-01-14 Thread Qiang Huang 

Signed-off-by: Qiang Huang 
---
 src/lxc/cgroup.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 6d837f9..69910cc 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -1669,7 +1669,11 @@ bool is_valid_cgroup(const char *name)
 {
const char *p;
for (p = name; *p; p++) {
-   if (*p < 32 || *p == 127 || *p == '/')
+   /* Use the ASCII printable characters range(32 - 127)
+* is reasonable, we kick out 32(SPACE) because it'll
+* break legacy lxc-ls
+*/
+   if (*p <= 32 || *p >= 127 || *p == '/')
return false;
}
return strcmp(name, ".") != 0 && strcmp(name, "..") != 0;
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] lxccontainer.c: check lxc_conf before referance haltsignal

2014-01-14 Thread Qiang Huang 
If we start container with rcfile(see comments in lxc_start.c), it
is possible that we have no config file in /usr/local/var/lib/lxc.
So when we try lxc_stop, lxc_container_new will not load any config
so we'll get c->lxc_conf = NULL.

In that case, we'll get Segmentation fault in lxcapi_shutdown, a
simple check would fix this.

Signed-off-by: Qiang Huang 
---
 src/lxc/lxccontainer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
index 8462ba5..0bebdff 100644
--- a/src/lxc/lxccontainer.c
+++ b/src/lxc/lxccontainer.c
@@ -1313,7 +1313,7 @@ static bool lxcapi_shutdown(struct lxc_container *c, int 
timeout)
pid = c->init_pid(c);
if (pid <= 0)
return true;
-   if (c->lxc_conf->haltsignal)
+   if (c->lxc_conf && c->lxc_conf->haltsignal)
haltsignal = c->lxc_conf->haltsignal;
kill(pid, haltsignal);
retv = c->wait(c, "STOPPED", timeout);
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] lxc-ls.sgml.in: fix the format issue

2014-01-08 Thread Qiang Huang 

Signed-off-by: Qiang Huang 
---
 doc/lxc-ls.sgml.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/lxc-ls.sgml.in b/doc/lxc-ls.sgml.in
index 2c8d5ae..90d0aaf 100644
--- a/doc/lxc-ls.sgml.in
+++ b/doc/lxc-ls.sgml.in
@@ -185,7 +185,9 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, 
MA 02110-1301 USA
 1
   ,
 
+  

+  
 Examples
 
   
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] configure.ac: add docbook-to-man to dbparsers

2014-01-07 Thread Qiang Huang 
On 2014/1/7 16:07, KATOH Yasufumi wrote:
> Hi,
> 
>>>> On Mon, 6 Jan 2014 20:00:29 +0800
> in message   "Re: [lxc-devel] [PATCH] configure.ac: add docbook-to-man to 
> dbparsers"
>   Qiang Huang-san wrote:
> 
>> This patch works fine for ja man page in my box, do you get any
>> real error messages?
> 
> Oh... sorry. I understand.

That's alright. :)

> 
>> PS: I use docbook2x-0.8.8-47.15 on SUSE11-sp2.
> 
> docbook-to-man is command name, isn't it? I confuse docbook-to-man

Yes, it's a command name, from docbook2x package, which is a
different name from the one for Debian/ubuntu.

> command in docbook2x package of (open)SuSE with docbook-to-man
> software(http://www.oasis-open.org/docbook/tools/dtm/) ;-)
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] configure.ac: add docbook-to-man to dbparsers

2014-01-06 Thread Qiang Huang 
Hi KATON,

On 2014/1/6 18:14, KATOH Yasufumi wrote:
> Hi,
> 
>>>> On Mon, 6 Jan 2014 10:53:15 +0800
> in message   "[lxc-devel] [PATCH] configure.ac: add docbook-to-man to 
> dbparsers"
>   Qiang Huang-san wrote:
> 
>> Debian and Ubuntu uses docbook2x-man, but some other distr like suse
>> uses docbook-to-man. I think all of them should work on LXC.
> 
> I guess that docbook-to-man cannot process the sgml that have UTF-8
> multibyte character. So this patch is likely to cause error in doc/ja.

This patch works fine for ja man page in my box, do you get any
real error messages?

PS: I use docbook2x-0.8.8-47.15 on SUSE11-sp2.

> 
> When docbook-to-man is used, it may need that doc/ja is excluded from
> target.
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] lxc-start-ephemeral: fix the man page

2014-01-05 Thread Qiang Huang 

Signed-off-by: Qiang Huang 
---
 doc/lxc-start-ephemeral.sgml.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/lxc-start-ephemeral.sgml.in b/doc/lxc-start-ephemeral.sgml.in
index 46e0592..d37ecf5 100644
--- a/doc/lxc-start-ephemeral.sgml.in
+++ b/doc/lxc-start-ephemeral.sgml.in
@@ -188,7 +188,9 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, 
MA 02110-1301 USA
 1
   ,
 
+  

+  
 Examples
 
   
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] configure.ac: add docbook-to-man to dbparsers

2014-01-05 Thread Qiang Huang 
Debian and Ubuntu uses docbook2x-man, but some other distr like suse
uses docbook-to-man. I think all of them should work on LXC.

Signed-off-by: Qiang Huang 
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 2d24937..576d210 100644
--- a/configure.ac
+++ b/configure.ac
@@ -127,7 +127,7 @@ AC_ARG_ENABLE([doc],

 if test "x$enable_doc" = "xyes" -o "x$enable_doc" = "xauto"; then
db2xman=""
-   dbparsers="docbook2x-man db2x_docbook2man docbook2man"
+   dbparsers="docbook2x-man db2x_docbook2man docbook2man docbook-to-man"

AC_MSG_CHECKING(for docbook2x-man)
for name in ${dbparsers}; do
-- 
1.8.3

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] lxc-test-concurrent: initialize saveptr before use

2013-12-10 Thread Qiang Huang 
Or we'll get this build error:
...
cc1: warnings being treated as errors
concurrent.c: In function ‘main’:
concurrent.c:165: error: ‘saveptr’ may be used uninitialized in this
function
make[2]: *** [concurrent.o] Error 1

Signed-off-by: Qiang Huang 
---
 src/tests/concurrent.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/tests/concurrent.c b/src/tests/concurrent.c
index b9749df..7145d99 100644
--- a/src/tests/concurrent.c
+++ b/src/tests/concurrent.c
@@ -162,7 +162,7 @@ int main(int argc, char *argv[]) {
 quiet = 1;
 break;
 case 'm': {
-char *mode_tok, *tok, *saveptr;
+char *mode_tok, *tok, *saveptr = NULL;

 modes = NULL;
 for (i = 0, mode_tok = optarg;
-- 
1.8.3


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] How to run autotests for LXC?

2013-12-10 Thread Qiang Huang 
On 2013/12/10 18:54, Nikola Kotur wrote:
> On Mon, 9 Dec 2013 15:25:18 +0800
> Qiang Huang  wrote:
> 
>> I was trying to play with the test code in LXC, which is
>> src/tests fold to be specific. But I just didn't find any
>> instructions.
> 
> First, enable the tests with the configure script:
> 
>   ./configure --enable-tests

Oh, I missed this '--enable-tests', thanks Nikola.

Now I have these lxc-test-xxx files in src/tests folder, do I
have to run them one by one, or is there any automatic way?

> 
> Then, build them:
> 
>   make check
> 
> I am not sure how up-to-date are they, though.
> 
> 
> 
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] How to run autotests for LXC?

2013-12-09 Thread Qiang Huang 
On 2013/12/10 1:38, Ranjib Dey wrote:
> did you try running ./configure script inside the parent directory (i.e. repo 
> root). 
> i am able to run `make check` inside the src folder after running the 
> ./configure 
> script in the parent directory.

Yes, I did.
And when I run 'make check' inside the src folder, it just make lxc folder, for 
the
other folders(including tests), still do nothing.

...
gcc -I../../src -DLXCROOTFSMOUNT=\"/usr/local/lib/lxc/rootfs\" 
-DLXCPATH=\"/usr/local/var/lib/lxc\" 
-DLXC_GLOBAL_CONF=\"/usr/local/etc/lxc/lxc.conf\" 
-DLXCINITDIR=\"/usr/local/libexec\"
-DLXCTEMPLATEDIR=\"/usr/local/share/lxc/templates\" 
-DLOGPATH=\"/usr/local/var/log/lxc\" 
-DLXC_DEFAULT_CONFIG=\"/usr/local/etc/lxc/default.conf\" 
-DLXC_USERNIC_DB=\"/run/lxc/nics\"
-DLXC_USERNIC_CONF=\"/usr/local/etc/lxc/lxc-usernet\" 
-DDEFAULT_CGROUP_PATTERN=\"/lxc/%n\" -DHAVE_APPARMOR-g -O2 -Wall -Werror 
-Wl,-E -Wl,-rpath -Wl,/usr/local/lib  -o lxc-init lxc_init.o
liblxc.so -lcap -lapparmor  -lrt -lutil -lapparmor
make[1]: Leaving directory `/home/h00177757/lxc-dev/src/lxc'
Making check in tests
make[1]: Entering directory `/home/h00177757/lxc-dev/src/tests'
make[1]: Nothing to be done for `check'.
make[1]: Leaving directory `/home/h00177757/lxc-dev/src/tests'
Making check in python-lxc
make[1]: Entering directory `/home/h00177757/lxc-dev/src/python-lxc'
make[1]: Nothing to be done for `check'.
make[1]: Leaving directory `/home/h00177757/lxc-dev/src/python-lxc'
Making check in lua-lxc
...

> 
> 
> On Sun, Dec 8, 2013 at 11:25 PM, Qiang Huang  <mailto:h.huangqi...@huawei.com>> wrote:
> 
> Hi list,
> 
> I was trying to play with the test code in LXC, which is
> src/tests fold to be specific. But I just didn't find any
> instructions.
> 
> I tried:
> # cd src/tests
> # make check
> make: Nothing to be done for `check'.
> 
> What do I miss? I think there should be a way we can build
> and run tests automatically, right?
> 
> 
> 
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org 
> <mailto:lxc-devel@lists.linuxcontainers.org>
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 
> 
> 
> 
> ___
> lxc-devel mailing list
> lxc-devel@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
> 


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] How to run autotests for LXC?

2013-12-08 Thread Qiang Huang 
Hi list,

I was trying to play with the test code in LXC, which is
src/tests fold to be specific. But I just didn't find any
instructions.

I tried:
# cd src/tests
# make check
make: Nothing to be done for `check'.

What do I miss? I think there should be a way we can build
and run tests automatically, right?



___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel