* Dhaval Giani <[email protected]> [2009-01-02 17:55:13]: > Some of the cleanups possible are obvious. > 1. Change usage of strcat to strncat > 2. Change usage of tge following type ^^^ the > char *s = malloc(); > strcpy(s, "somestring"); > strcat(s, "someotherstring"); > > to something more easily understandble such as > asprintf(&s, "%s%s", somestring, someotherstring); > > Changes from v1: > 1. Correct the error handling of asprintf as pointed out by Dan Smith >
We should document some of this in our coding style document, who wants to start writing it? :) > TODO: > 1. Figure out what is the correct value of n for cg_build_path_locked > > Signed-off-by: Dhaval Giani <[email protected]> > > --- > api.c | 101 > +++++++++++++++++++++++++++++++++++++++++------------------------- > 1 file changed, 64 insertions(+), 37 deletions(-) > > Index: trunk/api.c > =================================================================== > --- trunk.orig/api.c 2008-12-29 18:05:00.000000000 +0530 > +++ trunk/api.c 2009-01-02 14:21:37.000000000 +0530 > @@ -573,8 +573,7 @@ int cgroup_init() > &hierarchy, &num_cgroups, &enabled); > if (err < 0) > break; > - controllers[i] = (char *)malloc(strlen(subsys_name) + 1); > - strcpy(controllers[i], subsys_name); > + controllers[i] = strdup(subsys_name); > i++; > } > controllers[i] = NULL; > @@ -637,6 +636,11 @@ int cgroup_init() > > unlock_exit: > pthread_rwlock_unlock(&cg_mount_table_lock); > + > + for (i = 0; controllers[i] && i < CG_CONTROLLER_MAX; i++) { > + free(controllers[i]); > + controllers[i] = NULL; > + } Did you find a new leak, is that why you added this? Who was freeing controllers[] earlier? > return ret; > } > > @@ -689,12 +693,13 @@ static char *cg_build_path_locked(char * > { > int i; > for (i = 0; cg_mount_table[i].name[0] != '\0'; i++) { > + /* > + * XX: Change to snprintf once you figure what n should be > + */ > if (strcmp(cg_mount_table[i].name, type) == 0) { > - strcpy(path, cg_mount_table[i].path); > - strcat(path, "/"); > + sprintf(path, "%s/", cg_mount_table[i].path); > if (name) { Remove the extra brace as well, please > - strcat(path, name); > - strcat(path, "/"); > + sprintf(path, "%s%s/", path, name); > } > return path; > } > @@ -737,7 +742,7 @@ int cgroup_attach_task_pid(struct cgroup > if (!cg_build_path_locked(NULL, path, > cg_mount_table[i].name)) > continue; > - strcat(path, "/tasks"); > + strncat(path, "/tasks", sizeof(path) - strlen(path)); > One more to add to TODO list, abstract out common string functions into a small string library. > tasks = fopen(path, "w"); > if (!tasks) { > @@ -783,7 +788,7 @@ int cgroup_attach_task_pid(struct cgroup > cgroup->controller[i]->name)) > continue; > > - strcat(path, "/tasks"); > + strncat(path, "/tasks", sizeof(path) - strlen(path)); > > tasks = fopen(path, "w"); > if (!tasks) { > @@ -947,7 +952,7 @@ static int cg_set_control_value(char *pa > while (*(path+len) != '/') > len--; > *(path+len+1) = '\0'; > - strcat(path, "tasks"); > + strncat(path, "tasks", sizeof(path) - strlen(path)); > control_file = fopen(path, "r"); > if (!control_file) { > if (errno == ENOENT) > @@ -977,9 +982,10 @@ static int cg_set_control_value(char *pa > > int cgroup_modify_cgroup(struct cgroup *cgroup) > { > - char path[FILENAME_MAX], base[FILENAME_MAX]; > + char *path, base[FILENAME_MAX]; > int i; > int error; > + int ret; > > if (!cgroup_initialized) > return ECGROUPNOTINITIALIZED; > @@ -995,24 +1001,32 @@ int cgroup_modify_cgroup(struct cgroup * > } > } > > - strcpy(path, base); > - for (i = 0; i < cgroup->index; i++, strcpy(path, base)) { > + for (i = 0; i < cgroup->index; i++) { > int j; > if (!cg_build_path(cgroup->name, base, > cgroup->controller[i]->name)) > continue; > - strcpy(path, base); > - for (j = 0; j < cgroup->controller[i]->index; > - j++, strcpy(path, base)) { > - strcat(path, cgroup->controller[i]->values[j]->name); > + for (j = 0; j < cgroup->controller[i]->index; j++) { > + ret = asprintf(&path, "%s%s", base, > + cgroup->controller[i]->values[j]->name); We allocate in a loop, where do we free on error(), does the user explictly call cgroup_destroy_cgroup to free? > + if (ret < 0) { > + error = ECGOTHER; > + goto err; > + } > error = cg_set_control_value(path, > cgroup->controller[i]->values[j]->value); > + free(path); > + path = NULL; > if (error) > goto err; > } > } > + if (path) > + free(path); > return 0; > err: > + if (path) > + free(path); > return error; > > } > @@ -1099,10 +1113,11 @@ err: > */ > int cgroup_create_cgroup(struct cgroup *cgroup, int ignore_ownership) > { > - char *fts_path[2], base[FILENAME_MAX], *path; > + char *fts_path[2], *base = NULL, *path; > int i, j, k; > int error = 0; > int retval = 0; > + int ret; > > if (!cgroup_initialized) > return ECGROUPNOTINITIALIZED; > @@ -1127,8 +1142,6 @@ int cgroup_create_cgroup(struct cgroup * > * data structure. If not, we fail. > */ > for (k = 0; k < cgroup->index; k++) { > - path[0] = '\0'; > - > if (!cg_build_path(cgroup->name, path, > cgroup->controller[k]->name)) > continue; > @@ -1137,7 +1150,12 @@ int cgroup_create_cgroup(struct cgroup * > if (error) > goto err; > > - strcpy(base, path); > + base = strdup(path); > + > + if (!base) { > + error = ECGOTHER; > + goto err; > + } > > if (!ignore_ownership) > error = cg_chown_recursive(fts_path, > @@ -1146,9 +1164,14 @@ int cgroup_create_cgroup(struct cgroup * > if (error) > goto err; > > - for (j = 0; j < cgroup->controller[k]->index; j++, > - strcpy(path, base)) { > - strcat(path, cgroup->controller[k]->values[j]->name); > + for (j = 0; j < cgroup->controller[k]->index; j++) { > + free(path); > + ret = asprintf(&path, "%s%s", base, > + cgroup->controller[k]->values[j]->name); > + if (ret < 0) { > + error = ECGOTHER; > + goto err; > + } > error = cg_set_control_value(path, > cgroup->controller[k]->values[j]->value); > /* > @@ -1167,19 +1190,26 @@ int cgroup_create_cgroup(struct cgroup * > } > > if (!ignore_ownership) { > - strcpy(path, base); > - strcat(path, "/tasks"); > + free(path); > + ret = asprintf(&path, "%s/tasks", base); > + if (ret < 0) { > + error = ECGOTHER; > + goto err; > + } > error = chown(path, cgroup->tasks_uid, > cgroup->tasks_gid); > if (error) { > - error = ECGFAIL; > + error = ECGOTHER; > goto err; > } > } > } > > err: > - free(path); > + if (path) > + free(path); > + if (base) > + free(base); > if (retval && !error) > error = retval; > return error; > @@ -1314,7 +1344,7 @@ int cgroup_delete_cgroup(struct cgroup * > if (!cg_build_path(cgroup->name, path, > cgroup->controller[i]->name)) > continue; > - strcat(path, "../tasks"); > + strncat(path, "../tasks", sizeof(path) - strlen(path)); > > base_tasks = fopen(path, "w"); > if (!base_tasks) > @@ -1326,7 +1356,7 @@ int cgroup_delete_cgroup(struct cgroup * > continue; > } > > - strcat(path, "tasks"); > + strncat(path, "tasks", sizeof(path) - strlen(path)); > > delete_tasks = fopen(path, "r"); > if (!delete_tasks) { > @@ -1380,7 +1410,7 @@ static int cg_rd_ctrl_file(char *subsys, > if (!cg_build_path_locked(cgroup, path, subsys)) > return ECGFAIL; > > - strcat(path, file); > + strncat(path, file, sizeof(path) - strlen(path)); > ctrl_file = fopen(path, "r"); > if (!ctrl_file) > return ECGROUPVALUENOTEXIST; > @@ -1433,7 +1463,7 @@ static int cgroup_fill_cgc(struct dirent > */ > > cg_build_path_locked(cgroup->name, path, cg_mount_table[index].name); > - strcat(path, d_name); > + strncat(path, d_name, sizeof(path) - strlen(path)); > > error = stat(path, &stat_buffer); > > @@ -1492,6 +1522,7 @@ int cgroup_get_cgroup(struct cgroup *cgr > struct dirent *ctrl_dir; > char *control_path; > int error; > + int ret; > > if (!cgroup_initialized) { > /* ECGROUPNOTINITIALIZED */ > @@ -1537,17 +1568,13 @@ int cgroup_get_cgroup(struct cgroup *cgr > * Get the uid and gid information > */ > > - control_path = malloc(strlen(path) + strlen("tasks") + 1); > + ret = asprintf(&control_path, "%s/tasks", path); > > - if (!control_path) { > + if (ret < 0) { > error = ECGOTHER; > goto unlock_error; > } > > - strcpy(control_path, path); > - > - strcat(control_path, "tasks"); > - > if (stat(control_path, &stat_buffer)) { > free(control_path); > error = ECGOTHER; > > > This is quite a large change. I am not against large refactoring, but please post test results to build confidence in the changes. Please also include valgrind output if possible to show that no new leaks were added as a side-effect. -- Balbir ------------------------------------------------------------------------------ _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
