On Fri, Jan 02, 2009 at 09:46:16PM +0530, Balbir Singh wrote: > * 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
Will fix. > > 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? :) > /me suggests Balbir ;). I will try to get to it once I clear my table of the TODOs. > > 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? > You are right. This was a memory leak. I did not realize this when I was writing the code. I just freed it up since I had strduped it. > > 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 > Will do. > > - 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. > Added to the TODO list. > > 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? > Well, for one we free immediately after set_control_value, and if we fail, see err:, if path exists, we free it up. > > + 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. > Sure, let me add the valgrind output. The problem is that the test cases test a lot of the paths, but there is a lot of noise generated by the libcgrouptests, and so it is hard to figure out if it is due to the library, or due to the test cases. I think I should also add the test cases cleanup to my TODO list as well :). thanks for the review -- regards, Dhaval ------------------------------------------------------------------------------ _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
