* Sudhir Kumar <[email protected]> [2009-01-02 17:02:01]:
> A lack of proper initial thought let me use some variables globaly, which
> is not a good idea at all. However fixing this issue at this stage is not
> very good from the point that it changes variables for a lot of functions.
> But still it has to be done for a better and reusable code. This patch does
> that while putting related variables in meaningful structs.
> Thanks to Balbir for a good suggestion.
>
> Signed-off-by: Sudhir Kumar <[email protected]>
>
> ---
> tests/libcgrouptest.h | 48 ++++++++++-------
> tests/libcgrouptest01.c | 131
> ++++++++++++++++++++++++++----------------------
> 2 files changed, 101 insertions(+), 78 deletions(-)
>
> Index: trunk/tests/libcgrouptest.h
> ===================================================================
> --- trunk.orig/tests/libcgrouptest.h
> +++ trunk/tests/libcgrouptest.h
> @@ -34,8 +34,6 @@
> #define PASS 1 /* test passed */
> #define FAIL 0 /* test failed */
>
> -int cpu = 0, memory = 0;
> -
> enum cgroup_mount_t {
> FS_NOT_MOUNTED,
> FS_MOUNTED,
> @@ -78,26 +76,36 @@ enum info_message_t {
> NOMESSAGE,
> };
>
> +/* Keep a single struct of all ids */
> +struct uid_gid_t {
> + uid_t control_uid;
> + gid_t control_gid;
> + uid_t tasks_uid;
> + gid_t tasks_gid;
> +};
> +
> +/* Keep a single struct of all control values */
> +struct cntl_val_t {
> + int64_t val_int64;
> + u_int64_t val_uint64;
> + bool val_bool;
> + /* size worth of 100 digit num is fair enough */
> + char val_string[100]; /* string value of control parameter */
> +};
> +
> +extern int cpu, memory;
> +
> /* The set of verbose messages useful to the user */
> extern char info[NUM_MSGS][SIZE];
>
> -int64_t val_int64;
> -u_int64_t val_uint64;
> -bool val_bool;
> -/* Doubt: size of following string. is'nt this wrong ?*/
> -char val_string[FILENAME_MAX]; /* string value of control parameter */
> -uid_t control_uid;
> -gid_t control_gid;
> -uid_t tasks_uid;
> -gid_t tasks_gid;
> /* this variable is never modified */
> -int fs_mounted;
> +extern int fs_mounted;
>
> /* The mountpoints as received from script */
> -char mountpoint[FILENAME_MAX], mountpoint2[FILENAME_MAX];
> +extern char mountpoint[FILENAME_MAX], mountpoint2[FILENAME_MAX];
>
> /* No extra message unless specified */
> -char extra[SIZE] = "\n";
> +extern char extra[SIZE];
>
> /* Functions to test each API */
> void test_cgroup_init(int retcode, int i);
> @@ -105,7 +113,7 @@ void test_cgroup_attach_task(int retcode
> const char *group1, const char *group2,
> int k, int i);
> struct cgroup *create_new_cgroup_ds(int ctl, const char *grpname,
> - int value_type, int i);
> + int value_type, struct cntl_val_t cval, struct uid_gid_t ids, int i);
> void test_cgroup_create_cgroup(int retcode, struct cgroup *cgrp,
> const char *name, int common, int mpnt, int ign, int i);
> void test_cgroup_delete_cgroup(int retcode, struct cgroup *cgrp,
> @@ -113,7 +121,7 @@ void test_cgroup_delete_cgroup(int retco
> void test_cgroup_modify_cgroup(int retcode, struct cgroup *cgrp,
> const char *name, int which_ctl, int ctl1, int ctl2,
> int value_type, int i);
> -void test_cgroup_get_cgroup(int ctl1, int ctl2, int i);
> +void test_cgroup_get_cgroup(int ctl1, int ctl2, struct uid_gid_t ids, int i);
> /* API test functions end here */
>
> void test_cgroup_compare_cgroup(int ctl1, int ctl2, int i);
> @@ -122,11 +130,13 @@ void get_controllers(const char *name, i
> static int group_exist(char *path_group);
> static int set_controller(int controller, char *controller_name,
> char *control_file);
> -static int group_modified(char *path_control_file, int value_type);
> +static int group_modified(char *path_control_file, int value_type,
> + struct cntl_val_t cval);
> static int add_control_value(struct cgroup_controller *newcontroller,
> - char * control_file, char *wr, int value_type);
> + char *control_file, char *wr, int value_type, struct cntl_val_t cval);
> struct cgroup *new_cgroup(char *group, char *controller_name,
> - char *control_file, int value_type, int i);
> + char *control_file, int value_type, struct cntl_val_t cval,
> + struct uid_gid_t ids, int i);
> int check_fsmounted(int multimnt);
> static int check_task(char *tasksfile);
> /* function to print messages in better format */
> Index: trunk/tests/libcgrouptest01.c
> ===================================================================
> --- trunk.orig/tests/libcgrouptest01.c
> +++ trunk/tests/libcgrouptest01.c
> @@ -40,9 +40,17 @@ char info[NUM_MSGS][SIZE] = {
> "\n", /* NOMESSAGE */
> };
>
> +
> +int cpu, memory;
> +int fs_mounted;
> +char mountpoint[FILENAME_MAX], mountpoint2[FILENAME_MAX];
mountpoint2 is for multi-mount, comments would be useful?
> +char extra[SIZE] = "\n";
Some comments on what extra is for as well?
> +
> int main(int argc, char *argv[])
> {
> int retval;
> + struct uid_gid_t ids = {0}; /* Set default control permissions */
> + struct cntl_val_t cval = {2000, 2000, 1, "2000"};
As discussed earlier in the next iteration, it would be nice to see
these values come from a configuration file.
> struct cgroup *cgroup1, *cgroup2, *cgroup3, *nullcgroup = NULL;
> struct cgroup_controller *sec_controller;
> /* In case of multimount for readability we use the controller name
> @@ -90,12 +98,6 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> - /* Set default control permissions */
> - control_uid = 0;
> - control_gid = 0;
> - tasks_uid = 0;
> - tasks_gid = 0;
> -
> /*
> * Testsets: Testcases are broadly devided into 3 categories based on
> * filesystem(fs) mount scenario. fs not mounted, fs mounted, fs multi
> @@ -126,7 +128,8 @@ int main(int argc, char *argv[])
> * Exp outcome: no error
> */
>
> - cgroup1 = create_new_cgroup_ds(0, "group1", STRING, 3);
> + cgroup1 = create_new_cgroup_ds(0, "group1",
> + STRING, cval, ids, 3);
>
> /*
> * Test04: Then Call cgroup_create_cgroup() with this valid grp
> @@ -212,12 +215,13 @@ int main(int argc, char *argv[])
> * Test05: Create a valid cgroup structure
> * Exp outcome: no error. 0 return value
> */
> - cgroup1 = create_new_cgroup_ds(ctl1, "group1", STRING, 5);
> + cgroup1 = create_new_cgroup_ds(ctl1, "group1",
> + STRING, cval, ids, 5);
> if (!cgroup1) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Trying with second controller\n");
> cgroup1 = create_new_cgroup_ds(ctl2, "group1", STRING,
> - 5);
> + cval, ids, 5);
> if (!cgroup1) {
> fprintf(stderr, "Failed to create cgroup ds. "
> "Tests dependent on this structure "
> @@ -249,8 +253,8 @@ int main(int argc, char *argv[])
> build_path(path_control_file, mountpoint,
> "group1", control_file);
> retval = cgroup_modify_cgroup(cgroup1);
> - /* Check if the values are changed */
> - if (!retval && !group_modified(path_control_file, STRING))
> + /* Check if the values are changed. cval contains orig values */
> + if (!retval && !group_modified(path_control_file, STRING, cval))
> message(8, PASS, "modify_cgroup()", retval,
> info[SAMEGRP]);
> else
> @@ -261,12 +265,13 @@ int main(int argc, char *argv[])
> * Create another valid cgroup structure with same group
> * to modify the existing group
> */
> - cgroup2 = create_new_cgroup_ds(ctl1, "group1", STRING, 9);
> + cgroup2 = create_new_cgroup_ds(ctl1, "group1",
> + STRING, cval, ids, 9);
> if (!cgroup2) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Trying with second controller\n");
> - cgroup2 = create_new_cgroup_ds(ctl2, "group1", STRING,
> - 9);
> + cgroup2 = create_new_cgroup_ds(ctl2, "group1",
> + STRING, cval, ids, 9);
> if (!cgroup2) {
> fprintf(stderr, "Failed to create cgroup ds. "
> "Tests dependent on this structure "
> @@ -296,8 +301,9 @@ int main(int argc, char *argv[])
> * Create another valid cgroup structure with diff controller
> * to modify the existing group
> */
> - val_int64 = 262144;
> - cgroup3 = create_new_cgroup_ds(ctl2, "group1", INT64, 12);
> + cval.val_int64 = 262144;
> + cgroup3 = create_new_cgroup_ds(ctl2, "group1",
> + INT64, cval, ids, 12);
> if (!cgroup3) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -321,7 +327,7 @@ int main(int argc, char *argv[])
> * that "group1" exists in fs. So call cgroup_create_cgroup()
> * with "group1" named group before calling this test function.
> */
> - test_cgroup_get_cgroup(ctl1, ctl2, 14);
> + test_cgroup_get_cgroup(ctl1, ctl2, ids, 14);
>
> /*
> * Test16: delete cgroup
> @@ -391,7 +397,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> ctl1_cgroup1 = create_new_cgroup_ds(ctl1, "ctl1_group1",
> - STRING, 3);
> + STRING, cval, ids, 3);
> if (!ctl1_cgroup1) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -411,7 +417,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> ctl2_cgroup1 = create_new_cgroup_ds(ctl2, "ctl2_group1",
> - STRING, 5);
> + STRING, cval, ids, 5);
> if (!ctl2_cgroup1) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -459,7 +465,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> ctl2_cgroup2 = create_new_cgroup_ds(ctl2, "ctl2_group2",
> - STRING, 10);
> + STRING, cval, ids, 10);
> if (!ctl2_cgroup2) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -481,7 +487,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> mod_ctl1_cgroup1 = create_new_cgroup_ds(ctl1, "ctl1_group1",
> - STRING, 12);
> + STRING, cval, ids, 12);
> if (!mod_ctl1_cgroup1) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -502,7 +508,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> mod_ctl2_cgroup1 = create_new_cgroup_ds(ctl2, "ctl2_group1",
> - STRING, 14);
> + STRING, cval, ids, 14);
> if (!mod_ctl2_cgroup1) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -537,7 +543,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> common_cgroup = create_new_cgroup_ds(ctl1, "commongroup",
> - STRING, 18);
> + STRING, cval, ids, 18);
> if (!common_cgroup) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -582,7 +588,7 @@ int main(int argc, char *argv[])
> * Exp outcome: no error. 0 return value
> */
> mod_common_cgroup = create_new_cgroup_ds(ctl1, "commongroup",
> - STRING, 21);
> + STRING, cval, ids, 21);
> if (!common_cgroup) {
> fprintf(stderr, "Failed to create new cgroup ds. "
> "Tests dependent on this structure "
> @@ -607,9 +613,9 @@ int main(int argc, char *argv[])
> exit(1);
> }
>
> - strncpy(val_string, "7000064", sizeof(val_string));
> + strncpy(cval.val_string, "7000064", sizeof(cval.val_string));
> retval = cgroup_add_value_string(sec_controller,
> - control_file, val_string);
> + control_file, cval.val_string);
> if (retval)
> printf("The cgroup_modify_cgroup() test will fail\n");
>
> @@ -626,7 +632,7 @@ int main(int argc, char *argv[])
> */
> test_cgroup_delete_cgroup(0, common_cgroup,
> "commongroup", 1, 2, 1, 23);
> - test_cgroup_get_cgroup(ctl1, ctl2, 24);
> + test_cgroup_get_cgroup(ctl1, ctl2, ids, 24);
>
> /* Free the cgroup structures */
> cgroup_free(&nullcgroup);
> @@ -707,7 +713,7 @@ void test_cgroup_attach_task(int retcode
>
>
> struct cgroup *create_new_cgroup_ds(int ctl, const char *grpname,
> - int value_type, int i)
> + int value_type, struct cntl_val_t cval, struct uid_gid_t ids, int i)
> {
> int retval;
> char group[FILENAME_MAX];
> @@ -720,16 +726,15 @@ struct cgroup *create_new_cgroup_ds(int
> return NULL;
> }
>
> - /* val_string is still global. Will replace soon with config file */
> switch (ctl) {
> /* control values are controller specific, so will be set
> * accordingly from the config file */
> case CPU:
> - strncpy(val_string, "260000", sizeof(val_string));
> + strncpy(cval.val_string, "260000", sizeof(cval.val_string));
> break;
>
> case MEMORY:
> - strncpy(val_string, "7000064", sizeof(val_string));
> + strncpy(cval.val_string, "7000064", sizeof(cval.val_string));
> break;
>
> /* To be added for other controllers */
> @@ -740,7 +745,8 @@ struct cgroup *create_new_cgroup_ds(int
> break;
> }
>
> - return new_cgroup(group, controller_name, control_file, value_type, i);
> + return new_cgroup(group, controller_name, control_file,
> + value_type, cval, ids, i);
> }
>
>
> @@ -868,6 +874,7 @@ void test_cgroup_modify_cgroup(int retco
> int ctl2, int value_type, int i)
> {
> int retval;
> + struct cntl_val_t cval;
> char path1_control_file[FILENAME_MAX], path2_control_file[FILENAME_MAX];
> char controller_name[FILENAME_MAX], control_file[FILENAME_MAX];
>
> @@ -899,9 +906,9 @@ void test_cgroup_modify_cgroup(int retco
> set_controller(ctl1, controller_name, control_file);
> build_path(path1_control_file, mountpoint, name, control_file);
> /* this approach will be changed in coming patches */
> - strncpy(val_string, "260000", sizeof(val_string));
> + strncpy(cval.val_string, "260000", sizeof(cval.val_string));
>
> - if (!group_modified(path1_control_file, value_type))
> + if (!group_modified(path1_control_file, value_type, cval))
> message(i, PASS, "modify_cgroup()", retval,
> info[NOMESSAGE]);
> else
> @@ -921,8 +928,8 @@ void test_cgroup_modify_cgroup(int retco
> name, control_file);
>
> /* this approach will be changed in coming patches */
> - strncpy(val_string, "7000064", sizeof(val_string));
> - if (!group_modified(path2_control_file, value_type))
> + strncpy(cval.val_string, "7000064", sizeof(cval.val_string));
> + if (!group_modified(path2_control_file, value_type, cval))
> message(i, PASS, "modify_cgroup()", retval,
> info[NOMESSAGE]);
> else
> @@ -947,10 +954,12 @@ void test_cgroup_modify_cgroup(int retco
> name, control_file);
> }
> /* this approach will be changed in coming patches */
> - strncpy(val_string, "260000", sizeof(val_string));
> - if (!group_modified(path1_control_file, value_type)) {
> - strncpy(val_string, "7000064", sizeof(val_string));
> - if (!group_modified(path2_control_file, value_type))
> + strncpy(cval.val_string, "260000", sizeof(cval.val_string));
> + if (!group_modified(path1_control_file, value_type, cval)) {
> + strncpy(cval.val_string, "7000064",
> + sizeof(cval.val_string));
> + if (!group_modified(path2_control_file,
> + value_type, cval))
> message(i, PASS, "modify_cgroup()",
> retval, info[GRPMODINBOTHCTLS]);
> else
> @@ -1035,7 +1044,8 @@ static int set_controller(int controller
> }
> }
>
> -static int group_modified(char *path_control_file, int value_type)
> +static int group_modified(char *path_control_file, int value_type,
> + struct cntl_val_t cval)
> {
> bool bool_val;
> int64_t int64_val;
> @@ -1054,22 +1064,22 @@ static int group_modified(char *path_con
>
> case BOOL:
> fscanf(fd, "%d", &bool_val);
> - if (bool_val == val_bool)
> + if (bool_val == cval.val_bool)
> return 0;
> break;
> case INT64:
> fscanf(fd, "%lld", &int64_val);
> - if (int64_val == val_int64)
> + if (int64_val == cval.val_int64)
> return 0;
> break;
> case UINT64:
> fscanf(fd, "%llu", &uint64_val);
> - if (uint64_val == val_uint64)
> + if (uint64_val == cval.val_uint64)
> return 0;
> break;
> case STRING:
> fscanf(fd, "%s", string_val);
> - if (!strncmp(string_val, val_string, strlen(string_val)))
> + if (!strncmp(string_val, cval.val_string, strlen(string_val)))
> return 0;
> break;
> default:
> @@ -1082,7 +1092,7 @@ static int group_modified(char *path_con
> return 1;
> }
> static int add_control_value(struct cgroup_controller *newcontroller,
> - char * control_file, char *wr, int value_type)
> + char *control_file, char *wr, int value_type, struct cntl_val_t cval)
> {
> int retval;
>
> @@ -1090,22 +1100,22 @@ static int add_control_value(struct cgro
>
> case BOOL:
> retval = cgroup_add_value_bool(newcontroller,
> - control_file, val_bool);
> + control_file, cval.val_bool);
> snprintf(wr, SIZE, "add_value_bool()");
> break;
> case INT64:
> retval = cgroup_add_value_int64(newcontroller,
> - control_file, val_int64);
> + control_file, cval.val_int64);
> snprintf(wr, SIZE, "add_value_int64()");
> break;
> case UINT64:
> retval = cgroup_add_value_uint64(newcontroller,
> - control_file, val_uint64);
> + control_file, cval.val_uint64);
> snprintf(wr, SIZE, "add_value_uint64()");
> break;
> case STRING:
> retval = cgroup_add_value_string(newcontroller,
> - control_file, val_string);
> + control_file, cval.val_string);
> snprintf(wr, SIZE, "add_value_string()");
> break;
> default:
> @@ -1117,7 +1127,8 @@ static int add_control_value(struct cgro
> }
>
> struct cgroup *new_cgroup(char *group, char *controller_name,
> - char *control_file, int value_type, int i)
> + char *control_file, int value_type,
> + struct cntl_val_t cval, struct uid_gid_t ids, int i)
> {
> int retval;
> char wr[SIZE]; /* Names of wrapper apis */
> @@ -1127,8 +1138,8 @@ struct cgroup *new_cgroup(char *group, c
> newcgroup = cgroup_new_cgroup(group);
>
> if (newcgroup) {
> - retval = cgroup_set_uid_gid(newcgroup, tasks_uid, tasks_gid,
> - control_uid, control_gid);
> + retval = cgroup_set_uid_gid(newcgroup, ids.tasks_uid,
> + ids.tasks_gid, ids.control_uid, ids.control_gid);
>
> if (retval) {
> snprintf(wr, SIZE, "set_uid_gid()");
> @@ -1139,7 +1150,7 @@ struct cgroup *new_cgroup(char *group, c
> controller_name);
> if (newcontroller) {
> retval = add_control_value(newcontroller,
> - control_file, wr, value_type);
> + control_file, wr, value_type, cval);
>
> if (!retval) {
> message(i++, PASS, "new_cgroup()",
> @@ -1259,6 +1270,7 @@ static inline void build_path(char *targ
> void test_cgroup_compare_cgroup(int ctl1, int ctl2, int i)
> {
> int retval;
> + struct cntl_val_t cval = {0, 0, 0, "5000"};
> struct cgroup *cgroup1, *cgroup2;
> struct cgroup_controller *controller;
> char controller_name[FILENAME_MAX], control_file[FILENAME_MAX];
> @@ -1280,7 +1292,7 @@ void test_cgroup_compare_cgroup(int ctl1
> controller = cgroup_add_controller(cgroup1, controller_name);
> if (controller) {
> retval = add_control_value(controller,
> - control_file, wr, STRING);
> + control_file, wr, STRING, cval);
> if (retval)
> message(i++, FAIL, wr, retval, extra);
> }
> @@ -1288,7 +1300,7 @@ void test_cgroup_compare_cgroup(int ctl1
> controller = cgroup_add_controller(cgroup2, controller_name);
> if (controller) {
> retval = add_control_value(controller,
> - control_file, wr, STRING);
> + control_file, wr, STRING, cval);
> if (retval)
> message(i++, FAIL, wr, retval, extra);
> }
> @@ -1304,7 +1316,7 @@ void test_cgroup_compare_cgroup(int ctl1
> controller = cgroup_add_controller(cgroup2, controller_name);
> if (controller) {
> retval = add_control_value(controller,
> - control_file, wr, STRING);
> + control_file, wr, STRING, cval);
> if (retval)
> message(i++, FAIL, wr, retval, extra);
> }
> @@ -1319,11 +1331,12 @@ void test_cgroup_compare_cgroup(int ctl1
> cgroup_free(&cgroup2);
> }
>
> -void test_cgroup_get_cgroup(int ctl1, int ctl2, int i)
> +void test_cgroup_get_cgroup(int ctl1, int ctl2, struct uid_gid_t ids, int i)
> {
> struct cgroup *cgroup_filled, *cgroup_a, *cgroup_b;
> struct cgroup_controller *controller;
> char controller_name[FILENAME_MAX], control_file[FILENAME_MAX];
> + struct cntl_val_t cval = {0, 0, 0, "5000"};
> int ret;
>
> /*
> @@ -1371,7 +1384,7 @@ void test_cgroup_get_cgroup(int ctl1, in
> /* MULTIMOUNT: Create, get and compare a cgroup under both mounts */
>
> /* get cgroup_a ds and create group_a in filesystem */
> - cgroup_a = create_new_cgroup_ds(ctl1, "group_a", STRING, 00);
> + cgroup_a = create_new_cgroup_ds(ctl1, "group_a", STRING, cval, ids, 0);
> if (fs_mounted == FS_MULTI_MOUNTED) {
> /* Create under another controller also */
> ret = set_controller(ctl2, controller_name, control_file);
>
>
>
Otherwise, seems reasonable
Acked-by: Balbir Singh <[email protected]>
--
Balbir
------------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It is the best place to buy or sell services for
just about anything Open Source.
http://p.sf.net/sfu/Xq1LFB
_______________________________________________
Libcg-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libcg-devel
