Ah, I forgot the test case, and I seem to have messed that patch up. Let me fix that and resend in a few hours.
On Mon, Jul 26, 2010 at 5:50 AM, Balbir Singh <[email protected]> wrote: > On Mon, Jul 26, 2010 at 2:56 AM, Dhaval Giani <[email protected]> wrote: >> >> >> This patch adds a new API to get a list of procs. This is >> guaranteed to be sorted. >> >> TODO: >> 1. Ensure only unique values make it through >> >> Signed-off-by: Dhaval Giani <[email protected]> >> --- >> include/libcgroup/groups.h | 10 +++++ >> src/api.c | 86 >> +++++++++++++++++++++++++++++++++++++++++++++ >> src/libcgroup.map | 7 +++ >> 3 files changed, 103 insertions(+) >> >> Index: libcg/src/api.c >> =================================================================== >> --- libcg.orig/src/api.c >> +++ libcg/src/api.c >> @@ -3386,3 +3386,89 @@ int cgroup_get_all_controller_begin(void >> >> return cgroup_get_all_controller_next(handle, info); >> } >> + >> +static int pid_compare(const void *a, const void *b) >> +{ >> + const pid_t *pid1, *pid2; >> + >> + pid1 = (pid_t *) a; >> + pid2 = (pid_t *) b; >> + >> + if (*pid1 < *pid2) >> + return -1; >> + >> + if (*pid1 > *pid2) >> + return 1; > > nit-picking: Can't we just return *pid1 - *pid2? > :-). Now that you point it out, its so much more obvious. >> + >> + return 0; >> +} >> + >> +/* >> + *pids needs to be completely uninitialized so that we can set it up >> + * >> + * Caller must free up pids. >> + */ >> +int cgroup_get_procs(char *name, char *controller, pid_t **pids, int *size) >> +{ >> + char cgroup_path[FILENAME_MAX]; >> + FILE *procs; >> + pid_t *tmp_list; >> + int tot_procs = 1; > > I'd recommend lets start with a larger default like 8 or 16, so that > we don't have to do two reallocs for a group with 2 or 5 pids. > Yeah, I was wondering about a good default value. I will move it to 16. >> + int n = 0; >> + >> + cg_build_path(name, cgroup_path, controller); >> + strncat(cgroup_path, "/cgroup.procs", >> FILENAME_MAX-strlen(cgroup_path)); >> + >> + /* >> + * Read all the procs and then sort them up. >> + */ >> + >> + tmp_list = *pids; >> + >> + /* >> + * Keep doubling the memory allocated if needed >> + */ >> + tmp_list= malloc(sizeof(pid_t) * tot_procs); >> + if (!tmp_list) { >> + last_errno = errno; >> + return ECGOTHER; >> + } >> + >> + procs = fopen(cgroup_path, "r"); >> + if (!procs) { >> + last_errno = errno; >> + return ECGOTHER; >> + } > > This typically implies that we are using an older version of the > kernel, could you please add a comment indicating that. I wonder if > ENOTSUPP is a better error to return here > I should probably check if the file exists first (since we could fail opening for other reasons). Let me make that change. >> + >> + while (!feof(procs)) { >> + while (n < tot_procs) { >> + pid_t pid; >> + fscanf(procs, "%u", &pid); >> + tmp_list[n] = pid; >> + n++; > > This loop needs a feof() check as well > oh, i thought i had fixed this one (testing brought this issue up. Might have fixed it in the debug patch). >> + } >> + if (!feof(procs)) { >> + tot_procs *= 2; >> + tmp_list = realloc(tmp_list, sizeof(pid_t) * >> tot_procs); >> + if (!tmp_list) { >> + last_errno = errno; >> + return ECGOTHER; >> + } >> + } >> + } >> + >> + /* >> + * We decrement n because for some reason the last value >> + * gets repeated twice. Still need to investigate why, and >> + * if we are losing the last (sorted?) value, this is the >> + * obvious place to check. >> + */ > > Please see the comments above > No. not really. That did not help. Which is why the long comment. >> + n--; >> + *size = n; >> + >> + qsort(tmp_list, n, sizeof(pid_t), &pid_compare); > > Is there are reason we qsort here? I though the kernel already sorted > the pids.. no? > I have not seen a case where it is not sorted, but as per documentation (Documentation/cgroups/cgroups.txt in the kernel) - cgroup.procs: list of tgids in the cgroup. This list is not guaranteed to be sorted or free of duplicate tgids, and userspace should sort/uniquify the list if this property is required. This is a read-only file, for now. >> + >> + *pids = tmp_list; >> + > > Thanks for working on this > 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
