On Mon, Dec 29, 2008 at 11:38:23AM +0530, Balbir Singh wrote:
> * Dhaval Giani <[email protected]> [2008-12-28 19:51:29]:
>
> > cgroup_get_cgroup was failing with the memory controller enabled. It was
> > failing on memory.force_empty. The file has read permissions, but there
> > is no read routine associated with it inside the kernel. fscanf failed
> > and so cgroup_get_cgroup also failed. This was unexpected,
> > cgroup_get_cgroup should have just skipped the file. In order to fix
> > this, change cg_rd_ctrl_file to return more descriptive errors.
> >
> > Sigend-off-by: Dhaval Giani <[email protected]>
> >
> > ---
> > api.c | 29 +++++++++++++----------------
> > 1 file changed, 13 insertions(+), 16 deletions(-)
> >
> > Index: trunk/api.c
> > ===================================================================
> > --- trunk.orig/api.c 2008-12-22 14:18:26.000000000 +0530
> > +++ trunk/api.c 2008-12-28 02:11:14.000000000 +0530
> > @@ -1371,38 +1371,37 @@ open_err:
> > * will assume that the callers have taken care of everything.
> > * Including the locking.
> > */
> > -static char *cg_rd_ctrl_file(char *subsys, char *cgroup, char *file)
> > +static int cg_rd_ctrl_file(char *subsys, char *cgroup, char *file, char
> > **value)
> > {
> > - char *value;
> > char path[FILENAME_MAX];
> > FILE *ctrl_file;
> > int ret;
> >
> > if (!cg_build_path_locked(cgroup, path, subsys))
> > - return NULL;
> > + return ECGFAIL;
> >
> > strcat(path, file);
> > ctrl_file = fopen(path, "r");
> > if (!ctrl_file)
> > - return NULL;
> > + return ECGROUPVALUENOTEXIST;
> >
> > - value = malloc(CG_VALUE_MAX);
> > - if (!value)
> > - return NULL;
> > + *value = malloc(CG_VALUE_MAX);
> > + if (!*value)
> > + return ECGOTHER;
> >
> > /*
> > * using %as crashes when we try to read from files like
> > * memory.stat
> > */
> > - ret = fscanf(ctrl_file, "%s", value);
> > + ret = fscanf(ctrl_file, "%s", *value);
> > if (ret == 0 || ret == EOF) {
> > - free(value);
> > - value = NULL;
> > + free(*value);
> > + *value = NULL;
> > }
> >
> > fclose(ctrl_file);
> >
> > - return value;
> > + return 0;
> > }
> >
> > /*
> > @@ -1461,12 +1460,10 @@ static int cgroup_fill_cgc(struct dirent
> > }
> >
> > if (strcmp(ctrl_name, cg_mount_table[index].name) == 0) {
> > - ctrl_value = cg_rd_ctrl_file(cg_mount_table[index].name,
> > - cgroup->name, ctrl_dir->d_name);
> > - if (!ctrl_value) {
> > - error = ECGFAIL;
> > + error = cg_rd_ctrl_file(cg_mount_table[index].name,
> > + cgroup->name, ctrl_dir->d_name, &ctrl_value);
> > + if (error || !ctrl_value) {
> > goto fill_error;
>
> We overwrite error below, that line should also be removed
>
I am not sure I get you. The code snippet I see is
error = cg_rd_ctrl_file(cg_mount_table[index].name,
cgroup->name, ctrl_dir->d_name, &ctrl_value);
if (error || !ctrl_value) {
goto fill_error;
if (cgroup_add_value_string(cgc, ctrl_dir->d_name,
ctrl_value)) {
error = ECGFAIL;
goto fill_error;
And its fine to overwrite error here, since if it has failed till then,
we abort. Sorry for sounding so dumb.
> > - }
> >
> > if (cgroup_add_value_string(cgc, ctrl_dir->d_name,
> > ctrl_value)) {
> >
>
> We need to add support for reading .stat files as well, time to
> unleash the plugin layer 2 API infrastructure. I know we have patches
> in progress, may be it is time for an RFC?
>
Yes, it is time, let me fine tune it more, and post for both CPU and
memory. They are quite unpolished, and will need a lot of review and
design comments though.
thanks,
--
regards,
Dhaval
------------------------------------------------------------------------------
_______________________________________________
Libcg-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libcg-devel
