On 07/29/2011 01:36 AM, Dhaval Giani wrote:
> [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.
>
Ok
>>>
>>>> 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?
>
No, look we check for the first char in endptr, according to manual this
case can occur only if strol() is fail, and if it's not fail (argv[i] is
a digit) we do stat() for real check (try to take information about
/proc/pid_folder).
>>>> /* 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.
>
Ok, I'll rewrire and send to you the new one patch after my small
vacation (just 1 week).
Thx
> 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
------------------------------------------------------------------------------
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
