On Tue, Jul 27, 2010 at 1:10 PM, Jan Safranek <[email protected]> wrote: > On 07/26/2010 10:49 AM, Dhaval Giani wrote: >> Split cgroup_init so that cgroup_init_state will get the table and >> store it in the local state. The next step will be separate out the >> APIs to use state and finally we will introduce a global state. >> >> Signed-off-by: Dhaval Giani<[email protected]> >> >> --- >> include/Makefile.am | 2 - >> include/libcgroup.h | 3 + >> include/libcgroup/deprecated.h | 26 ++++++++++++++++ >> include/libcgroup/init.h | 10 ++---- >> src/Makefile.am | 2 - >> src/api.c | 66 >> ++++++++++++++++++++++++----------------- >> src/deprecated-api.c | 66 >> +++++++++++++++++++++++++++++++++++++++++ >> src/libcgroup-internal.h | 7 ++++ >> src/libcgroup.map | 2 + >> 9 files changed, 148 insertions(+), 36 deletions(-) >> >> Index: libcg/src/api.c >> =================================================================== >> --- libcg.orig/src/api.c >> +++ libcg/src/api.c >> @@ -65,9 +65,6 @@ static __thread char errtext[MAXLEN]; >> /* Task command name length */ >> #define TASK_COMM_LEN 16 >> >> -/* Check if cgroup_init has been called or not. */ >> -static int cgroup_initialized; >> - >> /* Check if the rules cache has been loaded or not. */ >> static bool cgroup_rules_loaded; >> >> @@ -80,12 +77,6 @@ static struct cgroup_rule_list trl; >> /* Lock for the list of rules (rl) */ >> static pthread_rwlock_t rl_lock = PTHREAD_RWLOCK_INITIALIZER; >> >> -/* Namespace */ >> -__thread char *cg_namespace_table[CG_CONTROLLER_MAX]; >> - >> -pthread_rwlock_t cg_mount_table_lock = PTHREAD_RWLOCK_INITIALIZER; >> -struct cg_mount_table_s cg_mount_table[CG_CONTROLLER_MAX]; >> - >> const char const *cgroup_strerror_codes[] = { >> "Cgroup is not compiled in", >> "Cgroup is not mounted", >> @@ -637,15 +628,23 @@ unlock: >> return ret; >> } >> >> -/** >> - * cgroup_init(), initializes the MOUNT_POINT. >> - * >> - * This code is theoretically thread safe now. Its not really tested >> - * so it can blow up. If does for you, please let us know with your >> - * test case and we can really make it thread safe. >> - * >> - */ >> -int cgroup_init(void) >> +void cgroup_free_context(struct cgroup_context_s **cg_context) >> +{ >> + int i; >> + struct cgroup_context_s *context = *cg_context; >> + >> + for (i = 0; i< context->size; i++) { >> + if (context->cg_namespace[i]) { >> + free(context->cg_namespace[i]); >> + context->cg_namespace[i] = NULL; >> + } >> + } >> + >> + free(context); >> + *cg_context = NULL; >> +} >> + >> +int cgroup_init_context(struct cgroup_context_s **cgroup_context) >> { >> FILE *proc_mount = NULL; >> struct mntent *ent = NULL; >> @@ -665,7 +664,17 @@ int cgroup_init(void) >> char mntent_buffer[4 * FILENAME_MAX]; >> char *strtok_buffer = NULL; >> >> - pthread_rwlock_wrlock(&cg_mount_table_lock); >> + struct cgroup_context_s *cg_context = calloc(1, >> + sizeof(struct cgroup_context_s)); >> + >> + if (!cg_context) { >> + last_errno = errno; >> + ret = ECGOTHER; >> + goto unlock_exit; >> + } >> + >> + for (i = 0; i< CG_CONTROLLER_MAX; i++) >> + cg_context->cg_namespace[i] = NULL; >> >> proc_cgroup = fopen("/proc/cgroups", "re"); >> >> @@ -695,6 +704,7 @@ int cgroup_init(void) >> } >> free(buf); >> >> + i = 0; >> while (!feof(proc_cgroup)) { >> err = fscanf(proc_cgroup, "%s %d %d %d", subsys_name, >> &hierarchy,&num_cgroups,&enabled); >> @@ -742,19 +752,20 @@ int cgroup_init(void) >> duplicate = 0; >> for (j = 0; j< found_mnt; j++) { >> if (strncmp(mntopt, cg_mount_table[j].name, >> - FILENAME_MAX) == 0) { >> + FILENAME_MAX) == 0) { >> duplicate = 1; >> break; >> } >> } >> if (duplicate) { >> cgroup_dbg("controller %s is already mounted >> on %s\n", >> - mntopt, cg_mount_table[j].path); >> + mntopt, cg_context->mount[j].path); >> continue; >> } >> >> - strcpy(cg_mount_table[found_mnt].name, controllers[i]); >> - strcpy(cg_mount_table[found_mnt].path, ent->mnt_dir); >> + strcpy(cg_context->mount[found_mnt].name, >> + controllers[i]); >> + strcpy(cg_context->mount[found_mnt].path, >> ent->mnt_dir); >> cgroup_dbg("Found cgroup option %s, count %d\n", >> ent->mnt_opts, found_mnt); >> found_mnt++; >> @@ -797,15 +808,15 @@ int cgroup_init(void) >> free(temp_ent); >> >> if (!found_mnt) { >> - cg_mount_table[0].name[0] = '\0'; >> + cg_context->mount[0].name[0] = '\0'; >> ret = ECGROUPNOTMOUNTED; >> goto unlock_exit; >> } >> >> found_mnt++; >> - cg_mount_table[found_mnt].name[0] = '\0'; >> + cg_context->size = found_mnt; >> + *cgroup_context = cg_context; >> >> - cgroup_initialized = 1; >> >> unlock_exit: >> if (proc_cgroup) >> @@ -819,7 +830,8 @@ unlock_exit: >> controllers[i] = NULL; >> } >> >> - pthread_rwlock_unlock(&cg_mount_table_lock); >> + if (ret) >> + cgroup_free_context(&cg_context); > > maybe also *cgroup_context=NULL, just to be sure? > >> >> return ret; >> } >> Index: libcg/src/libcgroup-internal.h >> =================================================================== >> --- libcg.orig/src/libcgroup-internal.h >> +++ libcg/src/libcgroup-internal.h >> @@ -135,6 +135,8 @@ struct cgroup_context_s { >> unsigned size; >> }; >> >> +struct cgroup_context_s *cgroup_context; > > This should probably go to deprecated-api.c, we don't want anyone to use > it except the deprecated API. >
No, not realy. See the next patch, and you will see how I am using it right now. >> + >> /** >> * per thread errno variable, to be used when return code is ECGOTHER >> */ >> @@ -171,6 +173,11 @@ int cgroup_config_insert_into_mount_tabl >> int cgroup_config_insert_into_namespace_table(char *name, char >> *mount_point); >> void cgroup_config_cleanup_mount_table(void); >> void cgroup_config_cleanup_namespace_table(void); >> + >> +/* >> + * Deprecated Stuff >> + */ >> +extern int cgroup_initialized; >> __END_DECLS >> >> #endif >> Index: libcg/src/deprecated-api.c >> =================================================================== >> --- /dev/null >> +++ libcg/src/deprecated-api.c >> @@ -0,0 +1,66 @@ >> +/* >> + * Copyright Dhaval Giani, 2010 >> + * >> + * Author: Dhaval Giani<[email protected]> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of version 2.1 of the GNU Lesser General Public License >> + * as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it would be useful, but >> + * WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. >> + */ >> + >> +#include<libcgroup.h> >> +#include<libcgroup-internal.h> >> +#include<string.h> >> + >> +/* Check if cgroup_init has been called or not. */ >> +int cgroup_initialized; >> + >> +/* Namespace */ >> +__thread char *cg_namespace_table[CG_CONTROLLER_MAX]; >> + >> +pthread_rwlock_t cg_mount_table_lock = PTHREAD_RWLOCK_INITIALIZER; >> +struct cg_mount_table_s cg_mount_table[CG_CONTROLLER_MAX]; > > I understand that the three definitions above are for direct usage e.g. > by api.c, they are not deprecated. > >> +struct cgroup *context_cgroup; >> + >> +/** >> + * cgroup_init(), initializes the MOUNT_POINT. >> + * >> + * This code is theoretically thread safe now. Its not really tested >> + * so it can blow up. If does for you, please let us know with your >> + * test case and we can really make it thread safe. >> + * >> + * Deprecate this call in favour of keeping local state >> + */ >> +int cgroup_init(void) >> +{ >> + int ret = 0; >> + int i; >> + >> + pthread_rwlock_wrlock(&cg_mount_table_lock); >> + >> + ret = cgroup_init_context(&cgroup_context); >> + >> + if (ret) >> + goto out; >> + >> + /* Move everything from state into the global table >> + * Once the changes are complte, this code goes away. >> + * This is only for the APIs not converted yet. >> + */ >> + cgroup_initialized = 1; > > I am not sure I understand this correctly... If these 4 patches are > accepted, will applications using cgroup_init work? It looks like they > could. Will applications using cgroup_init_context work? I have bad > feeling about it - if the comment above is correct, they probably wont, > because global cg_mount_table is not filled. > the code below does just that ;-). Though you are right, I should move this to after it being filled up. >> + >> + for (i = 0; i< cgroup_context->size; i++) { >> + strcpy(cg_mount_table[i].name, cgroup_context->mount[i].name); >> + strcpy(cg_mount_table[i].path, cgroup_context->mount[i].path); >> + } It is being filled here right now. >> + cg_mount_table[i].name[0] = '\0'; >> + >> + pthread_rwlock_unlock(&cg_mount_table_lock); >> +out: >> + return ret; >> +} >> + >> Index: libcg/include/libcgroup/deprecated.h >> =================================================================== >> --- /dev/null >> +++ libcg/include/libcgroup/deprecated.h >> @@ -0,0 +1,26 @@ >> +#ifndef _LIBCGROUP_DEPRECATED_H >> +#define _LIBCGROUP_DEPRECATED_H >> + >> +#ifndef _LIBCGROUP_H_INSIDE >> +#error "Only<libcgroup.h> should be included directly." >> +#endif >> + >> +#include<features.h> >> + >> +__BEGIN_DECLS >> + >> +/** >> + * >> + * TODO: Extend docbook >> + * >> + */ >> + >> +/** >> + * Initialize libcgroup. Information about mounted hierarchies are examined >> + * and cached internally (just what's mounted where, not the groups >> themselves). >> + */ >> +int cgroup_init(void); >> + >> +__END_DECLS >> + >> +#endif >> Index: libcg/include/libcgroup/init.h >> =================================================================== >> --- libcg.orig/include/libcgroup/init.h >> +++ libcg/include/libcgroup/init.h >> @@ -30,11 +30,10 @@ __BEGIN_DECLS >> * on exit. >> */ >> >> -/** >> - * Initialize libcgroup. Information about mounted hierarchies are examined >> - * and cached internally (just what's mounted where, not the groups >> themselves). >> - */ >> -int cgroup_init(void); >> +struct cgroup_context_s; >> + >> +int cgroup_init_context(struct cgroup_context_s **cgroup_cgroup); >> +void cgroup_free_context(struct cgroup_context_s **cg_context); >> >> /** >> * Returns path where is mounted given controller. Applications should >> rely on >> @@ -50,7 +49,6 @@ int cgroup_get_subsys_mount_point(const >> * @} >> */ >> >> -struct cgroup_context_s; >> __END_DECLS >> >> #endif /* _LIBCGROUP_INIT_H */ >> Index: libcg/src/Makefile.am >> =================================================================== >> --- libcg.orig/src/Makefile.am >> +++ libcg/src/Makefile.am >> @@ -7,7 +7,7 @@ CLEANFILES = lex.c parse.c parse.h >> >> INCLUDES = -I$(top_srcdir)/include >> lib_LTLIBRARIES = libcgroup.la >> -libcgroup_la_SOURCES = parse.h parse.y lex.l api.c config.c >> libcgroup-internal.h libcgroup.map wrapper.c >> +libcgroup_la_SOURCES = parse.h parse.y lex.l api.c config.c >> libcgroup-internal.h libcgroup.map wrapper.c deprecated-api.c >> libcgroup_la_LIBADD = -lpthread >> libcgroup_la_LDFLAGS = -Wl,--version-script,$(srcdir)/libcgroup.map \ >> -version-number >> $(LIBRARY_VERSION_MAJOR):$(LIBRARY_VERSION_MINOR):$(LIBRARY_VERSION_RELEASE) >> Index: libcg/include/libcgroup.h >> =================================================================== >> --- libcg.orig/include/libcgroup.h >> +++ libcg/include/libcgroup.h >> @@ -19,11 +19,12 @@ >> #define _LIBCGROUP_H_INSIDE >> >> #include<libcgroup/error.h> >> -#include<libcgroup/init.h> >> #include<libcgroup/iterators.h> >> #include<libcgroup/groups.h> >> #include<libcgroup/tasks.h> >> #include<libcgroup/config.h> >> +#include<libcgroup/init.h> >> +#include<libcgroup/deprecated.h> >> >> #undef _LIBCGROUP_H_INSIDE >> >> Index: libcg/include/Makefile.am >> =================================================================== >> --- libcg.orig/include/Makefile.am >> +++ libcg/include/Makefile.am >> @@ -1,2 +1,2 @@ >> # Using 'nobase_', we what groups.h in/usr/include/libcgroup/ directory >> -nobase_include_HEADERS = libcgroup.h libcgroup/error.h libcgroup/init.h >> libcgroup/groups.h libcgroup/tasks.h libcgroup/iterators.h libcgroup/config.h >> +nobase_include_HEADERS = libcgroup.h libcgroup/error.h libcgroup/init.h >> libcgroup/groups.h libcgroup/tasks.h libcgroup/iterators.h >> libcgroup/config.h libcgroup/deprecated.h >> Index: libcg/src/libcgroup.map >> =================================================================== >> --- libcg.orig/src/libcgroup.map >> +++ libcg/src/libcgroup.map >> @@ -91,4 +91,6 @@ CGROUP_0.36 { >> >> CGROUP_0.37 { >> cgroup_get_procs; >> + cgroup_init_context; >> + cgroup_free_context; >> } CGROUP_0.36; >> Thanks for thre review ------------------------------------------------------------------------------ The Palm PDK Hot Apps Program offers developers who use the Plug-In Development Kit to bring their C/C++ apps to Palm for a share of $1 Million in cash or HP Products. Visit us here for more details: http://ad.doubleclick.net/clk;226879339;13503038;l? http://clk.atdmt.com/CRS/go/247765532/direct/01/ _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
