[Sorry for the delay with the reviews, been facing some issues with my laptop]
On Sun, Jul 24, 2011 at 8:43 PM, Nikiforov Alex <a.nikifo...@samsung.com> wrote:
> On 07/22/2011 06:11 PM, Michal Hocko wrote:
>> On Fri 22-07-11 08:53:53, Nikiforov Alex wrote:
>>> From f1390d329b1172bfc38b43bb11a56b8cb6db1af8 Mon Sep 17 00:00:00 2001
>>> From: Alex Nikiforov<nika@gentoo>
>>> Date: Thu, 21 Jul 2011 13:20:48 +0400
>>> Subject: [PATCH 1/2] cgclassify: add a PID check into the client
>>>
>>> Add PID check code to the client. We dont need any read()/write() if
>>> it's not valid.
>>>
>>> Signed-off-by: Alex Nikiforov<a.nikifo...@samsung.com>
>>> ---
>>> src/tools/cgclassify.c | 9 +++++++--
>>> 1 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/tools/cgclassify.c b/src/tools/cgclassify.c
>>> index 397b725..cffc63f 100644
>>> --- a/src/tools/cgclassify.c
>>> +++ b/src/tools/cgclassify.c
>>> @@ -121,7 +121,8 @@ int main(int argc, char *argv[])
>>> struct cgroup_group_spec *cgroup_list[CG_HIER_MAX];
>>> int c;
>>> char *endptr;
>>> -
>>> + char path[FILENAME_MAX];
>>> + struct stat buff_stat;
>>>
>>> if (argc< 2) {
>>> usage(1, argv[0]);
>>> @@ -167,8 +168,12 @@ int main(int argc, char *argv[])
>>> }
>>>
>>> for (i = optind; i< argc; i++) {
>>> +
>>
>> Why the empty line?
> just dont like tons of code, I think it's very user full to separate
> cycle head and the first operator. For me this style is more convenient
> when I read a code (this is just IMHO)
A lot of us are used to the kernel coding style, where we don't add
these extra lines. So yes, no whitespace here.
>>
>>> pid = (uid_t) strtol(argv[i],&endptr, 10);
>>> - if (endptr[0] != '\0') {
>>> + snprintf(path, FILENAME_MAX, "/proc/%d", pid);
>>> +
>>> + /* check PID */
>>> + if (endptr[0] != '\0' || stat(path,&buff_stat)) {
Shouldn't this always evaluate as true? Considering that the only way
we can enter this loop is on have endptr[0] != '\0'. IOW, why this
condition?
>>> /* the input argument was not a number */
>>> fprintf(stderr, "Error: %s is not valid pid.\n",
>>> argv[i]);
>>
>> I would prefer it like this:
>> if (!(*endptr))
>> goto bad_pid;
>> snprintf(path, FILENAME_MAX, "/proc/%d", pid);
>> if (stat(path,&buff_stat)) {
>> bad_pid:
>> fprintf(stderr, "Error: %s is not valid pid.\n",
>> argv[i]);
>>
>> exit_code = 2;
>> continue;
>> }
>>
>> Why should we do snprintf if we know that the given parameter was not a
>> number or it contains some garbage.
> I really dont like exception style coding in plain C. It's hard to read
> as well as hard to debug when you add some traces (this is IMHO too). We
> can add goto, but it's really ugly style here not so much work happen to
> add goto, snprint() is reenterant in this case it's fast. Dont see any
> point to add goto here.
>
Please follow this style. We have been following it, and it is always
better to have code in a uniform style.
Thanks!
Dhaval
------------------------------------------------------------------------------
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
_______________________________________________
Libcg-devel mailing list
Libcg-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libcg-devel
