On Tue, Jul 27, 2010 at 1:10 PM, Jan Safranek <[email protected]> wrote:
> On 07/26/2010 10:49 AM, Dhaval Giani wrote:
>> part of the series to convert to a thread aware as opposed to a
>> thread safe model.
>>
>> Signed-off-by: Dhaval Giani<[email protected]>
>> ---
>> include/libcgroup/deprecated.h | 12 +++++++
>> include/libcgroup/error.h | 1
>> include/libcgroup/groups.h | 3 -
>> src/api.c | 66
>> +++++++++++++++++++++++++----------------
>> src/libcgroup.map | 1
>> 5 files changed, 55 insertions(+), 28 deletions(-)
>>
>> Index: libcg/src/api.c
>> ===================================================================
>> --- libcg.orig/src/api.c
>> +++ libcg/src/api.c
>> @@ -1331,15 +1331,28 @@ int cgroup_create_cgroup(struct cgroup *
>> int error = 0;
>> int retval = 0;
>> int ret;
>> -
>> - if (!cgroup_initialized)
>> - return ECGROUPNOTINITIALIZED;
>> + struct cgroup_context_s *cg_context;
>> + int deprecated = 0;
>>
>> if (!cgroup)
>> return ECGROUPNOTALLOWED;
>>
>> + if (!cgroup->context) {
>> + pthread_rwlock_rdlock(&cg_mount_table_lock);
>> + cgroup->context = cgroup_context;
>> + deprecated = 1;
>> + cgroup_dbg("WARNING: This is deprecated. Please utilize the " \
>> + "new context aware interfaces\n");
>> + }
>
> How does an application set cgroup->context? I have not found any
> function for that, did I miss something?
>
As I mentioned in the 0 patch, this does need another wrapper API. I
am still testing it to ensure that stuff does not break yet.
> I think that it would be better to have
> cgroup_new_cgroup_ctx(struct cgroup_context_s *ctx, char *name), which
> creates the structure and puts the context there, and
When I did that, I was advised (by Lennart) that this is a better
method. That stuff is still online on that git repository.
> cgroup_new_cgroup(), which just calls
> cgroup_new_cgroup_ctx(the_global_context, name) (and optionally yells at
> user about deprecated API, uninitialized library etc.)
>
> cgroup_create_cgroup (and all other functions like delete,
> create_from_parent, compare, ...) just assume that the context is always
> filled, no extra handling is needed there.
>
The additional handling is not very expensive and quite
straightforward. And anyhow we would call back into this function with
the context and stuff. So, let's just not break stuff so badly, and
not lose the nice names as well. So I think it is a net win. However,
this can always be changed if I am convinced about it ;-).
> And I am not sure about _context suffix - if it is going to be heavily
> used, I would propose _ctx. If it is used only in
> cgroup_new_cgroup_context and cgroup_init_context then it's probably fine.
>
I don't even want to do that. Just the cgroup_init_context and
cgroup_attach_context sort of a thing.
>> +
>> + if (!cgroup->context->size)
>> + return ECGROUPINVALCONTEXT;
>> +
>> + cg_context = cgroup->context;
>> +
>> for (i = 0; i< cgroup->index; i++) {
>> - if (!cgroup_test_subsys_mounted(cgroup->controller[i]->name))
>> + if (!cgroup_test_subsys_context(cg_context,
>> + cgroup->controller[i]->name))
>> return ECGROUPSUBSYSNOTMOUNTED;
>> }
>>
>> @@ -1348,6 +1361,7 @@ int cgroup_create_cgroup(struct cgroup *
>> last_errno = errno;
>> return ECGOTHER;
>> }
>> +
>> fts_path[1] = NULL;
>> path = fts_path[0];
>>
>> @@ -1357,7 +1371,7 @@ int cgroup_create_cgroup(struct cgroup *
>> * data structure. If not, we fail.
>> */
>> for (k = 0; k< cgroup->index; k++) {
>> - if (!cg_build_path(cgroup->name, path,
>> + if (!cg_build_path_context(cg_context, cgroup->name, path,
>> cgroup->controller[k]->name))
>> continue;
>>
>> @@ -1366,19 +1380,16 @@ int cgroup_create_cgroup(struct cgroup *
>> goto err;
>>
>> base = strdup(path);
>> -
>> if (!base) {
>> last_errno = errno;
>> error = ECGOTHER;
>> goto err;
>> }
>>
>> - if (!ignore_ownership) {
>> - cgroup_dbg("Changing ownership of %s\n", fts_path[0]);
>> - error = cg_chown_recursive(fts_path,
>> - cgroup->control_uid, cgroup->control_gid);
>> - }
>> + cgroup_dbg("Changing ownership of %s\n", fts_path[0]);
>>
>> + error = cg_chown_recursive(fts_path,
>> + cgroup->control_uid, cgroup->control_gid);
>> if (error)
>> goto err;
>>
>> @@ -1392,6 +1403,7 @@ int cgroup_create_cgroup(struct cgroup *
>> error = ECGOTHER;
>> goto err;
>> }
>> +
>> error = cg_set_control_value(path,
>> cgroup->controller[k]->values[j]->value);
>> /*
>> @@ -1409,26 +1421,30 @@ int cgroup_create_cgroup(struct cgroup *
>> }
>> }
>>
>> - if (!ignore_ownership) {
>> - ret = snprintf(path, FILENAME_MAX, "%s/tasks", base);
>> - if (ret< 0 || ret>= FILENAME_MAX) {
>> - last_errno = errno;
>> - error = ECGOTHER;
>> - goto err;
>> - }
>> - error = chown(path, cgroup->tasks_uid,
>> - cgroup->tasks_gid);
>> - if (error) {
>> - last_errno = errno;
>> - error = ECGOTHER;
>> - goto err;
>> - }
>> + ret = snprintf(path, FILENAME_MAX, "%s/tasks", base);
>> + if (ret< 0 || ret>= FILENAME_MAX) {
>> + last_errno = errno;
>> + error = ECGOTHER;
>> + goto err;
>> }
>> +
>> + error = chown(path, cgroup->tasks_uid, cgroup->tasks_gid);
>> + if (error) {
>> + last_errno = errno;
>> + error = ECGOTHER;
>> + goto err;
>> + }
>> +
>> free(base);
>> base = NULL;
>> }
>>
>> err:
>> + if(deprecated) {
>> + cgroup->context = NULL;
>> + pthread_rwlock_unlock(&cg_mount_table_lock);
>> + }
>> +
>> if (path)
>> free(path);
>> if (base)
>> Index: libcg/include/libcgroup/error.h
>> ===================================================================
>> --- libcg.orig/include/libcgroup/error.h
>> +++ libcg/include/libcgroup/error.h
>> @@ -71,6 +71,7 @@ enum {
>> ECGNAMESPACEPATHS,
>> ECGNAMESPACECONTROLLER,
>> ECGMOUNTNAMESPACE,
>> + ECGROUPINVALCONTEXT,
>> };
>>
>> /**
>> Index: libcg/include/libcgroup/deprecated.h
>> ===================================================================
>> --- libcg.orig/include/libcgroup/deprecated.h
>> +++ libcg/include/libcgroup/deprecated.h
>> @@ -21,6 +21,18 @@ __BEGIN_DECLS
>> */
>> int cgroup_init(void);
>>
>> +/**
>> + * Physically create a control group in kernel. The group is created in all
>> + * hierarchies, which cover controllers added by cgroup_add_controller().
>> + * All parameters set by cgroup_add_value_* functions are written.
>> + * The created groups has owner which was set by cgroup_set_uid_gid().
>> + * @param cgroup
>> + * @param ignore_ownership When nozero, all errors are ignored when setting
>> + * owner of the group and/or its tasks file.
>> + * @todo what is ignore_ownership good for?
>> + */
>> +int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership);
>> +
>> __END_DECLS
>>
>> #endif
>> Index: libcg/include/libcgroup/groups.h
>> ===================================================================
>> --- libcg.orig/include/libcgroup/groups.h
>> +++ libcg/include/libcgroup/groups.h
>> @@ -172,9 +172,6 @@ void cgroup_free_controllers(struct cgro
>> * All parameters set by cgroup_add_value_* functions are written.
>> * The created groups has owner which was set by cgroup_set_uid_gid().
>> * @param cgroup
>> - * @param ignore_ownership When nozero, all errors are ignored when setting
>> - * owner of the group and/or its tasks file.
>> - * @todo what is ignore_ownership good for?
>> */
>> int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership);
>>
>> Index: libcg/src/libcgroup.map
>> ===================================================================
>> --- libcg.orig/src/libcgroup.map
>> +++ libcg/src/libcgroup.map
>> @@ -93,4 +93,5 @@ CGROUP_0.37 {
>> cgroup_get_procs;
>> cgroup_init_context;
>> cgroup_free_context;
>> + cgroup_create_cgroup_context;
>> } CGROUP_0.36;
>>
>>
Thanks for the review!
Dhaval
------------------------------------------------------------------------------
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
