On Wed, Dec 15, 2010 at 2:18 PM, Jan Safranek <jsafr...@redhat.com> wrote:
> On 12/15/2010 10:12 AM, Michal Hrusecky wrote:
>> Rules can be written using wildcards, but destinations had to be static.
>> This patch adds support for following strings in destination:
>>
>>     %u - uid
>>     %U - username, uid in case of error
>>     %g - gid
>>     %G - group name, gid in case of error
>>     %p - pid
>>     %P - proccess name, pid in case of error
>>
>> So more general rules can be specified using wildcards. Example rule can
>> be:
>>
>> *...@users        *     %G/%U
>>
>> This will put all users in their own cgroups named by their login and
>> group.
>
> I really like the idea, would you mind updating man page(s) as well?
>
>>
>> Signed-off-by: Michal Hrusecky<mic...@hrusecky.net>
>> ---
>>   src/api.c |   82 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 81 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/api.c b/src/api.c
>> index 859190a..29b8627 100644
>> --- a/src/api.c
>> +++ b/src/api.c
>> @@ -2366,6 +2366,12 @@ int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
>>       /* Temporary pointer to a rule */
>>       struct cgroup_rule *tmp = NULL;
>>
>> +     /* Temporary variables for destination substitution */
>> +     char newdest[FILENAME_MAX];
>> +     int i, j;
>> +     struct passwd * user_info;
>> +     struct group  * group_info;
>> +
>>       /* Return codes */
>>       int ret = 0;
>>
>> @@ -2418,7 +2424,81 @@ int cgroup_change_cgroup_flags(uid_t uid, gid_t gid,
>>       do {
>>               cgroup_dbg("Executing rule %s for PID %d... ", tmp->username,
>>                                                               pid);
>> -             ret = cgroup_change_cgroup_path(tmp->destination,
>> +             // Destination substitutions
>> +             for(j = i = 0; i<  strlen(tmp->destination); i++, j++) {
>> +                     if(tmp->destination[i] == '%') {
>> +                             switch(tmp->destination[++i]) {
>> +                                     case 'u':
>> +                                             j += snprintf(newdest+j,
>> +                                                     FILENAME_MAX-j,
>> +                                                     "%d", uid);
>> +                                             i++;
>> +                                             break;
>> +                                     case 'U':
>> +                                             user_info = getpwuid(uid);
>> +                                             if(user_info) {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%s",
>> +                                                             user_info ->
>> +                                                             pw_name);
>> +                                             } else {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%d", uid);
>> +                                             }
>> +                                             i++;
>> +                                             break;
>> +                                     case 'g':
>> +                                             j += snprintf(newdest+j,
>> +                                                     FILENAME_MAX-j,
>> +                                                     "%d", gid);
>> +                                             i++;
>> +                                             break;
>> +                                     case 'G':
>> +                                             group_info = getgrgid(gid);
>> +                                             if(group_info) {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%s",
>> +                                                             group_info ->
>> +                                                             gr_name);
>> +                                             } else {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%d", gid);
>> +                                             }
>> +                                             i++;
>> +                                             break;
>> +                                     case 'p':
>> +                                             j += snprintf(newdest+j,
>> +                                                     FILENAME_MAX-j,
>> +                                                     "%d", pid);
>> +                                             i++;
>> +                                             break;
>> +                                     case 'P':
>> +                                             if(procname) {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%s",
>> +                                                             procname);
>> +                                             } else {
>> +                                                     j += 
>> snprintf(newdest+j,
>> +                                                             FILENAME_MAX-j,
>> +                                                             "%d", pid);
>> +                                             }
>> +                                             i++;
>> +                                             break;
>> +                                     default:
>> +                                             newdest[j++] = '%';
>> +                             }
>> +                     }
>> +                     if(tmp->destination[i] == '\\')
>> +                             i++;
>> +                     newdest[j] = tmp->destination[i];
>
> The code here is... hairy. There is too much i++ and j++ for me. For
> example, consider a tmp->destination = "%G%U" without any character
> between. The "case 'G'" fills the group name, but the line right above
> this comment copies '%' without expansion of '%U'.
>

hehe. I was planning to give it a closer look to figure out where the
i's an the j's went. (Which is also why i said whether the asprintf
might be easier or not ;-) ). Well, I guess this part would need to be
rewritten or at the very least very well commented ;-).

> In addition, with tmp->destination = "<something long>%G/%U" and really
> long groupname exceeding FILENAME_MAX in newdest, the "case 'G'" fills
> up the newdest and the aforementioned line above writes one character
> *behind* the newdest. And expanding '%U' goes completely crazy,
> FILENAME_MAX-j gets negative, converted to (unsigned) size_t,
> overwriting who knows what.
>

I am assuming using asprintf should rid us of these issues. (Just
probably too many temporary variables though..)

Dhaval
------------------------------------------------------------------------------
Lotusphere 2011
Register now for Lotusphere 2011 and learn how
to connect the dots, take your collaborative environment
to the next level, and enter the era of Social Business.
http://p.sf.net/sfu/lotusphere-d2d
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel

Reply via email to