On Fri, Jan 30, 2009 at 10:15:43AM +0100, Ivana Varekova wrote:
> Hello,
> I'm just sending a patch which fixes a few error values in cgclassify
> command - there are others which have to be fixed too (on my TODO list
> now).
> Ivana Varekova
> diff -up ./api.c.orig ./api.c
> --- ./api.c.orig 2009-01-29 10:19:02.000000000 +0100
> +++ ./api.c 2009-01-29 10:06:05.000000000 +0100
> @@ -94,6 +94,8 @@ char *cgroup_strerror_codes[] = {
> "Cgroup parsing failed",
> "Cgroup, rules file does not exist",
> "Cgroup mounting failed",
> + "Process with given pid does not exist",
> + "euid or egid of the given proces can not be found",
I wonder if this could be a bit clearer. Maybe, expand it out as
Effective uid or effective gid, but I am a bit ambivalent here, and
will trust your judgement.
> };
>
> static int cg_chown_file(FTS *fts, FTSENT *ent, uid_t owner, gid_t group)
> diff -up ./cgclassify.c.orig ./cgclassify.c
> --- ./cgclassify.c.orig 2009-01-29 10:19:02.000000000 +0100
> +++ ./cgclassify.c 2009-01-29 10:08:06.000000000 +0100
> @@ -34,33 +34,33 @@
> * It returns 0 on success and negative values on failure.
> */
>
> -int euid_of_pid(pid_t pid)
> +int euid_of_pid(pid_t pid, uid_t *euid)
I am curious, why did you pass it as a pointer?
> {
> FILE *fp;
> char path[FILENAME_MAX];
> char buf[TEMP_BUF];
> - uid_t ruid, euid, suid, fsuid;
> + uid_t ruid, suid, fsuid;
>
> sprintf(path, "/proc/%d/status", pid);
> fp = fopen(path, "r");
> if (!fp) {
> - fprintf(stderr, "Error in opening file %s:%s\n", path,
> + dbg("Error in opening file %s:%s\n", path,
> strerror(errno));
this change should come in another patch
> - return -1;
> + return ECGPROCNEXISTS;
> }
>
> while (fgets(buf, TEMP_BUF, fp)) {
> if (!strncmp(buf, "Uid:", 4)) {
> sscanf((buf + 5), "%d%d%d%d", (int *)&ruid,
> - (int *)&euid, (int *)&suid, (int *)&fsuid);
> + (int *)euid, (int *)&suid, (int *)&fsuid);
> dbg("Scanned proc values are %d %d %d %d\n",
> - ruid, euid, suid, fsuid);
> - return euid;
> + ruid, *euid, suid, fsuid);
> + return 0;
> }
> }
>
> /* If we are here, we could not find euid. Return error. */
> - return -1;
> + return ECGEUIDGIDFAIL;
ok, looks good to me.
> }
>
> /*
> @@ -69,33 +69,33 @@ int euid_of_pid(pid_t pid)
> * It returns 0 on success and negative values on failure.
> */
>
> -int egid_of_pid(pid_t pid)
> +int egid_of_pid(pid_t pid, uid_t *egid)
> {
> FILE *fp;
> char path[FILENAME_MAX];
> char buf[TEMP_BUF];
> - gid_t rgid, egid, sgid, fsgid;
> + gid_t rgid, sgid, fsgid;
>
> sprintf(path, "/proc/%d/status", pid);
> fp = fopen(path, "r");
> if (!fp) {
> - fprintf(stderr, "Error in opening file %s:%s\n", path,
> + dbg("Error in opening file %s:%s\n", path,
> strerror(errno));
> - return -1;
> + return ECGPROCNEXISTS;
> }
>
> while (fgets(buf, TEMP_BUF, fp)) {
> if (!strncmp(buf, "Gid:", 4)) {
> sscanf((buf + 5), "%d%d%d%d", (int *)&rgid,
> - (int *)&egid, (int *)&sgid, (int *)&fsgid);
> + (int *)egid, (int *)&sgid, (int *)&fsgid);
> dbg("Scanned proc values are %d %d %d %d\n",
> - rgid, egid, sgid, fsgid);
> - return egid;
> + rgid, *egid, sgid, fsgid);
> + return 0;
> }
> }
>
> /* If we are here, we could not find egid. Return error. */
> - return -1;
> + return ECGEUIDGIDFAIL;
> }
>
> int main(int argc, char *argv[])
> @@ -122,17 +122,18 @@ int main(int argc, char *argv[])
> /* Put pids into right cgroups as per rules in /etc/cgrules.conf */
> for (i = 1; i < argc; i++) {
> pid = (uid_t) atoi(argv[i]);
> - euid = euid_of_pid(pid);
> - if (euid == -1) {
> + ret = euid_of_pid(pid, &euid);
> +
> + if (ret > 0) {
> fprintf(stderr, "Error in determining euid of"
> - " pid %d\n", pid);
> + " pid %d: %s\n", pid,
> cgroup_strerror(ret));
> return -1;
> }
>
> - egid = egid_of_pid(pid);
> - if (egid == -1) {
> + ret = egid_of_pid(pid, &egid);
> + if (ret > 0) {
> fprintf(stderr, "Error in determining egid of"
> - " pid %d\n", pid);
> + " pid %d: %s\n", pid,
> cgroup_strerror(ret));
> return -1;
> }
>
> diff -up ./libcgroup.h.orig ./libcgroup.h
> --- ./libcgroup.h.orig 2009-01-29 10:19:02.000000000 +0100
> +++ ./libcgroup.h 2009-01-29 10:05:41.000000000 +0100
> @@ -94,6 +94,8 @@ enum cgroup_errors {
> ECGROUPPARSEFAIL, /* Failed to parse rules configuration file. */
> ECGROUPNORULES, /* Rules list does not exist. */
> ECGMOUNTFAIL,
> + ECGPROCNEXISTS, /* Proces with given pid does not exist*/
> + ECGEUIDGIDFAIL, /* euid of the given proces can't be found */
> ECGSENTINEL, /* Please insert further error codes above this */
> };
>
So this patch looks good, apart from a couple of questions (mainly why
are you passing a pointer, I think a value is good enough). and ensuring
that you follow the DCO, and split the patch apart into two and provide
a proper changelog. With those changes and split, you can add a
Acked-by: Dhaval Giani <[email protected]>
Thanks Ivana!
--
regards,
Dhaval
------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
Libcg-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libcg-devel
