[lxc-devel] multiple cgroup instances fixes

[Serge Hallyn]

Quoting Daniel Lezcano (daniel.lezcano at free.fr):
> Hi Serge,
> 
> your patch was doing a nested call to the setmntent which is not reentrant.
> 
> Fixed with the patch in attachment. Some other nits fixed too.

Looks good, thanks.

Acked-by: Serge Hallyn <serge.hallyn at ubuntu.com>

Once this all settles down, we can of course consider re-introducing
caching the mounts we find so multiple lxc_cgroup_{sg}et()s are
quicker.  But I figured optimizations should be done later.

> ---
>  src/lxc/cgroup.c |   81 +++++++++++++++++++++----------------------------------
>  1 file changed, 32 insertions(+), 49 deletions(-)
> 
> Index: lxc/src/lxc/cgroup.c
> ===================================================================
> --- lxc.orig/src/lxc/cgroup.c
> +++ lxc/src/lxc/cgroup.c
> @@ -82,40 +82,19 @@ static int get_cgroup_mount(const char *
>          return -1;
>  }
>  
> -static int get_cgroup_flags(const char *mtab, const char *mnt_dir, int *flags)
> +static int get_cgroup_flags(const struct mntent *mntent)
>  {
> -        struct mntent *mntent;
> -        FILE *file = NULL;
> -        int err = -1;
> +	int flags = 0;
>  
> -        file = setmntent(mtab, "r");
> -        if (!file) {
> -                SYSERROR("failed to open %s", mtab);
> -		return -1;
> -        }
> +	if (hasmntopt(mntent, "ns"))
> +		flags |= CGROUP_NS_CGROUP;
>  
> -	*flags = 0;
>   -- Daniel

> ---
>  src/lxc/cgroup.c |   81 +++++++++++++++++++++----------------------------------
>  1 file changed, 32 insertions(+), 49 deletions(-)
> 
> Index: lxc/src/lxc/cgroup.c
> ===================================================================
> --- lxc.orig/src/lxc/cgroup.c
> +++ lxc/src/lxc/cgroup.c
> @@ -82,40 +82,19 @@ static int get_cgroup_mount(const char *
>          return -1;
>  }
>  
> -static int get_cgroup_flags(const char *mtab, const char *mnt_dir, int *flags)
> +static int get_cgroup_flags(const struct mntent *mntent)
>  {
> -        struct mntent *mntent;
> -        FILE *file = NULL;
> -        int err = -1;
> +	int flags = 0;
>  
> -        file = setmntent(mtab, "r");
> -        if (!file) {
> -                SYSERROR("failed to open %s", mtab);
> -		return -1;
> -        }
> +	if (hasmntopt(mntent, "ns"))
> +		flags |= CGROUP_NS_CGROUP;
>  
> -	*flags = 0;
> +	if (hasmntopt(mntent, "clone_children"))
> +		flags |= CGROUP_CLONE_CHILDREN;
>  
> -        while ((mntent = getmntent(file))) {
> -		if (strcmp(mntent->mnt_type, "cgroup"))
> -			continue;
> -		if (strcmp(mntent->mnt_dir, mnt_dir))
> -			continue;
> -		if (hasmntopt(mntent, "ns")) {
> -			*flags |= CGROUP_NS_CGROUP;
> -			err = 0;
> -		}
> -		if (hasmntopt(mntent, "clone_children")) {
> -			*flags |= CGROUP_CLONE_CHILDREN;
> -			err = 0;
> -		}
> -		fclose(file);
> -		DEBUG("cgroup flags for %s is 0x%x", mnt_dir, *flags);
> -		return err;
> -	}
> +	DEBUG("cgroup '%s' has flags 0x%x", mntent->mnt_dir, flags);
>  
> -	fclose(file);
> -        return err;
> +        return flags;
>  }
>  
>  static int cgroup_rename_nsgroup(const char *mnt, const char *name, pid_t pid)
> @@ -184,14 +163,15 @@ static int cgroup_attach(const char *pat
>   * XXX TODO we will of course want to use cgroup_path{subsystem}/lxc/name,
>   * not just cgroup_path{subsystem}/name.
>   */
> -int lxc_one_cgroup_create(const char *name, const char *cgmnt, pid_t pid)
> +static int lxc_one_cgroup_create(const char *name,
> +				 const struct mntent *mntent, pid_t pid)
>  {
>  	char cgname[MAXPATHLEN];
>  	char clonechild[MAXPATHLEN];
>  	int flags;
>  
> -	snprintf(cgname, MAXPATHLEN, "%s/%s", cgmnt, name);
> -
> +	snprintf(cgname, MAXPATHLEN, "%s/%s", mntent->mnt_dir, name);
> +
>  	/*
>  	 * There is a previous cgroup, assume it is empty,
>  	 * otherwise that fails
> @@ -201,18 +181,18 @@ int lxc_one_cgroup_create(const char *na
>  		return -1;
>  	}
>  
> -	if (get_cgroup_flags(MTAB, cgmnt, &flags)) {
> -		SYSERROR("failed to get cgroup flags");
> -		return -1;
> -	}
> +	flags = get_cgroup_flags(mntent);
>  
>  	/* We have the deprecated ns_cgroup subsystem */
>  	if (flags & CGROUP_NS_CGROUP) {
>  		WARN("using deprecated ns_cgroup");
> -		return cgroup_rename_nsgroup(cgmnt, cgname, pid);
> +		return cgroup_rename_nsgroup(mntent->mnt_dir, cgname, pid);
>  	}
>  
> -	snprintf(clonechild, MAXPATHLEN, "%s/cgroup.clone_children", cgmnt);
> +	snprintf(clonechild, MAXPATHLEN, "%s/cgroup.clone_children",
> +		 mntent->mnt_dir);
> +
> +	DEBUG("checking '%s' presence", clonechild);
>  
>  	/* we check if the kernel has clone_children, at this point if there
>  	 * no clone_children neither ns_cgroup, that means the cgroup is mounted
> @@ -242,6 +222,8 @@ int lxc_one_cgroup_create(const char *na
>  		return -1;
>  	}
>  
> +	INFO("created cgroup '%s'", cgname);
> +
>  	return 0;
>  }
>  
> @@ -252,9 +234,8 @@ int lxc_cgroup_create(const char *name,
>  {
>          struct mntent *mntent;
>          FILE *file = NULL;
> -        int ret, err = -1;
> +        int err = -1;
>  
> -	DEBUG("%s: starting\n", __func__);
>          file = setmntent(MTAB, "r");
>          if (!file) {
>                  SYSERROR("failed to open %s", MTAB);
> @@ -262,19 +243,21 @@ int lxc_cgroup_create(const char *name,
>  	}
>  
>          while ((mntent = getmntent(file))) {
> +
> +		DEBUG("checking '%s' (%s)", mntent->mnt_dir, mntent->mnt_type);
> +
>                  if (!strcmp(mntent->mnt_type, "cgroup")) {
> -			DEBUG("creating %s %s %d\n", name, mntent->mnt_dir, pid);
> -			err = lxc_one_cgroup_create(name, mntent->mnt_dir, pid);
> -			if (ret) {
> -				fclose(file);
> -				return ret;
> -			}
> -			err = 0;
> +
> +			INFO("found cgroup mounted at '%s'", mntent->mnt_dir);
> +			err = lxc_one_cgroup_create(name, mntent, pid);
> +			if (err)
> +				goto out;
> +
>  		}
>          };
>  
> -        fclose(file);
> -
> +out:
> +        endmntent(file);
>          return err;
>  }
>