On Wed, Nov 21, 2012 at 7:10 AM, Ivana Hutarova Varekova <
varek...@redhat.com> wrote:
> Template cgroups mean control groups which are set in cgrules.conf file
> and the name contains % variable like %U (see cgrules.conf manual page for
> the whole list of variables).
>
> This patch change cgroup_change_cgroup_flags function. Now if the wanted
> group is template group and the group does not exist then
> cgroup_change_cgroup_flags create the control group on the fly .
>
> For now the created group can't be set - there is always used function
> cgroup_create_cgroup. This will be changed in other patch in this patchset.
>
> EXAMPLE:
> e.g.
> @students devices people/students/%U
> cgroup_change_cgroup_flags will create a cgroup /people/students/john
> if user john from group students run a command and the people does not
> exist yet.
> if /people/students group does not exist it will be created as well
>
>
> Signed-off-by: Ivana Hutarova Varekova <varek...@redhat.com>
> ---
>
> src/api.c | 175
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 175 insertions(+), 0 deletions(-)
>
> diff --git a/src/api.c b/src/api.c
> index ea75cca..178c19e 100644
> --- a/src/api.c
> +++ b/src/api.c
> @@ -2635,6 +2635,174 @@ static struct cgroup_rule
> *cgroup_find_matching_rule(uid_t uid,
> return ret;
> }
>
> +/* Procedure the existence of cgroup "prefix" is in subsystem
> controller_name
> + * return 0 on success
> + */
> +int cgroup_exist_in_subsystem(char *controller_name, char *prefix)
> +{
> + DIR *dir;
> + char path[FILENAME_MAX];
> +
> + pthread_rwlock_rdlock(&cg_mount_table_lock);
> + if (!cg_build_path_locked(prefix, path, controller_name)) {
> + pthread_rwlock_unlock(&cg_mount_table_lock);
> + return 1;
> + }
> + pthread_rwlock_unlock(&cg_mount_table_lock);
> +
>
maybe i am being picky but
ret = cg_build_path_locked(prefix, path, controller_name);
phtread_rwlock_unlock(&cg_mount_table_lock)
if (!ret)
return 1;
(though I prefer a goto)
> + dir = opendir(path);
> + if (dir == NULL) {
> + /* cgroup in wanted subsystem does not exist */
> + return 1;
> + } else {
> + /* cgroup in wanted subsystem exists */
> + return 0;
> + closedir(dir);
> + }
> +}
> +
>
I really prefer a single return point as opposed to so many.
> +/* auxiliary function return a pointer to the string
> + * which is copy of input string and end with the backslash
> + */
>
Why is it needed? (also I think you meant slash :P)
> +char *cgroup_copy_with_backslash(char *input)
> +{
> + char *output;
> + int len = strlen(input);
> +
> + /* if input does not end with '/', allocate one more space for it
> */
> + if ((input[len-1]) != '/')
> + len = len+1;
>
+
> + output = (char *)malloc(sizeof(char)*(len));
> + if (output == NULL)
> + return NULL;
> +
> + strcpy(output, input);
> + output[len-1] = '/';
> + output[len] = '\0';
> +
> + return output;
> +}
> +
>
I don't like this function. You could exit earlier, which you don't. There
is also scope for leaks which I don't like. (Have you run this under
valgrind or something similar?). Also, can we make it static? That way we
can have a smaller name as well ;-)
> +/* create control group based given template
> + * if the group already don't exist
> + * dest is template name with substitute variables
> + * tmp is used cgrules rule
> + */
> +int cgroup_create_template_group(char *orig_group_name, struct
> cgroup_rule *tmp,
> + int flags)
> +{
> +
> + char *template_name = NULL; /* name of the template */
> + char *group_name = NULL; /* name of the group based on
> template -
> + variables are substituted */
> + char *template_position; /* denotes directory in template
> path
> + which is investigated */
> + char *group_position; /* denotes directory in cgroup path
> + which is investigated */
> +
> + struct cgroup *template_group = NULL;
> + int ret = 0;
> + int i;
> + int exist;
> + int first;
> + struct cgroup_controller *controller;
> +
> + /* template name and group name have to have '/' sign at the end */
> + template_name = cgroup_copy_with_backslash(tmp->destination);
> + if (template_name == NULL) {
> + ret = ECGOTHER;
> + goto end;
> + }
> + group_name = cgroup_copy_with_backslash(orig_group_name);
> + if (group_name == NULL) {
> + ret = ECGOTHER;
> + goto end;
> + }
> +
>
last_errno = errno ?
> + /* set start positions */
> + template_position = strchr(template_name, '/');
> + group_position = strchr(group_name, '/');
> +
>
Are we thread safe here?
> + /* go recursively through whole path to template group and create
> given
> + * directory if it does not exist yet
> + */
> + while ((group_position != NULL) && (template_position != NULL)) {
> + /* set new subpath */
> + group_position[0] = '\0';
> + template_position[0] = '\0';
> + first = 0;
> +
> + /* test for which controllers wanted group does not exist
> */
> + i = 0;
> + while (tmp->controllers[i] != NULL) {
> + exist =
> cgroup_exist_in_subsystem(tmp->controllers[i],
> + group_name);
> +
> + if (exist != 0) {
> + /* the cgroup does not exist */
> + if (first == 0) {
> + /* it is the first controller for
> which
> + the group does not exist */
> + first = 1;
> + template_group =
> +
> cgroup_new_cgroup(group_name);
> + if (template_group == NULL) {
> + ret = ECGFAIL;
> + goto end;
> + }
> + }
> +
> + controller = cgroup_add_controller(
> + template_group,
> tmp->controllers[i]);
> + if (!controller) {
> + cgroup_free(&template_group);
> + ret = ECGFAIL;
> + goto end;
> + }
> + }
> + i++;
> + }
>
Too many brackets :-). Can we split this into another function? (I lose
track of what happens?)
> +
> + if (first == 1) {
> + /* new group have to be created */
> + if (strcmp(group_name, template_name) == 0) {
> + /* the prefix cgroup without template */
> + ret = cgroup_create_cgroup(template_group,
> 0);
> + } else {
> + /* TODO: this will be a function which use
> + * template to create relevant cgroup
> + * now cgroup_create_cgroup is used
> + ret = cgroup_config_create_template_group(
> + template_group, template_name,
> + 0, flags);
> + */
> + ret = cgroup_create_cgroup(template_group,
> 0);
> + }
> +
> + if (ret != 0) {
> + cgroup_free(&template_group);
> + goto end;
> + }
> + cgroup_dbg("Group %s created - based on template
> %s\n",
> + group_name, template_name);
> +
> + cgroup_free(&template_group);
> + }
> + template_position[0] = '/';
> + group_position[0] = '/';
> + template_position = strchr(++template_position, '/');
> + group_position = strchr(++group_position, '/');
> + }
> +
> +end:
> + if (group_name != NULL)
> + free(group_name);
> + if (template_name != NULL)
> + free(template_name);
> + return ret;
> +}
> +
> int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
> const char *procname, pid_t pid, int flags)
> {
> @@ -2783,7 +2951,14 @@ int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
> newdest[j] = tmp->destination[i];
> }
> }
> +
> newdest[j] = 0;
> + if (strcmp(newdest, tmp->destination) != 0) {
> + /* destination tag contains templates */
> +
> + cgroup_dbg("control group %s is template\n",
> newdest);
> + ret = cgroup_create_template_group(newdest, tmp,
> flags);
> + }
>
> /* Apply the rule */
> ret = cgroup_change_cgroup_path(newdest,
>
>
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
