Forwarding to the list... -------- Original Message -------- Subject: libcgroup code review Date: Fri, 12 Nov 2010 14:55:48 -0500 From: Steve Grubb <[email protected]> To: [email protected]
Hello, I see that you are doing commits to upstream code on sourceforge. Below I have the results of a code review. This is against whatever was in the fedora master branch. -Steve In src/api.c *At line 196, there is a return without freeing tmp_string. *At line 637, is a return without freeing newrule. For this one, I would suggest setting newrule to NULL after assigning it to the linked list. This way you can have a free after the close label. *At line 2131 is a for loop that re-uses the index variable of the for loop at 2122. Should there be 2 variables one for each loop? *At line 2566 ret is checked to see if its not 3. The test for EOF is not needed because EOF is -1 which is not 3. *At line 2749 is a return without freeing "entry". *At line 3192 len=0; is never executed. All paths lead to a continue or a break. In src/config.c *At line 521 is a check for mount_path to be NULL. It never can because path is an array and not a pointer. In src/wrapper *At line 166 is return without freeing cntl_value *At line 203 is return without freeing cntl_value *At line 245 is return without freeing cntl_value *At line 445 ret is checked to see if its less than 0, however its unsigned. *At line 493 ret is checked to see if its less than 0, however its unsigned. *At line 552 ret is checked to see if its less than 0, however its unsigned. In tools/cgexec.c *At line 111, would using setresuid() be more safe? But as I think about it, maybe the unix_domain socket should have a group & 0660 on it and cgexec be a setgid app. In tools/cgcreate.c *At line 089 is a return without freeing cgroup_list *At line 100 is a return without freeing cgroup_list *At line 123 is a return without freeing cgroup_list *At line 145 is a return without freeing cgroup_list *At line 152 is a return without freeing cgroup_list In tools/cgget.c *At line 55, ret is checked to not be 0. I suspect that the call immediately above it should have assigned its return code to ret. iow, ret = cgroup_read_stats_begin(... *At line 358, ret is checked to be non-zero. However at line 353 this was already done. Should the call to display_all_controllers have assigned its return code to ret? ------------------------------------------------------------------------------ Centralized Desktop Delivery: Dell and VMware Reference Architecture Simplifying enterprise desktop deployment and management using Dell EqualLogic storage and VMware View: A highly scalable, end-to-end client virtualization framework. Read more! http://p.sf.net/sfu/dell-eql-dev2dev _______________________________________________ Libcg-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/libcg-devel
