February 8th, 2011
If you are a seasoned C programmer, skip this post entirely (or try to find bugs in it). If you know C but don't consider yourself an expert, please read on -- it might be helpful.
I was working a bit on vzctl today (my target was bug #1757, which is still a work in progress) and ... I am not sure how, but I ended up declaring most functions in src/vzlist.c as static. I thought it doesn't have any practical value -- I was wrong!
In C, if you declare the function as static, it means its visibility is limited to the translation unit (i.e. a file) in which it is defined. In other words, you can only call/use a static function from another function in the same file.
Now, in vzctl sources vzlist.c is only linked to one binary -- vzlist, and therefore I thought it doesn't make much sense to declare functions as static. Nevertheless I did it (see git commit).
Next thing I got is a set of compiler warnings! OK, all right, let's take a look...
First set of warnings is self-explanatory. See:
vzlist.c:825:14: warning: ‘parse_var’ defined but not used
vzlist.c:1075:14: warning: ‘remove_sp’ defined but not used
vzlist.c:1357:12: warning: ‘get_stop_quota_stats’ defined but not used
Easy! In some ancient time, these functions were used, now the code has changed and no one needs these three, but they were not removed for some reason (probably just forgotten). Solution: remove the dead code (see git commit).
Second set of warnings looks similar:
vzlist.c:400:1: warning: ‘dcachesize_m_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_l_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_b_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_f_sort_fn’ defined but not used
vzlist.c:411:1: warning: ‘diskinodes_s_sort_fn’ defined but not used
vzlist.c:411:1: warning: ‘diskinodes_h_sort_fn’ defined but not used
Hmm... all these *_sort_fn are sort functions generated by means of a few #define statements, and they are used when vzlist needs to sort its output by some column or parameter (vzlist -s). It is very strange that these are not used, because they should be. Let's take a closer look... zOMG! it's a bug!
Apparently, someone was using copy-paste technique* and forgot to change the names of the functions. The bug is, when you ask vzlist to sort its output to, say, dcachesize failcounter values, it sorts it by dcachesize held values instead, because of the wrong sort function used. Such bugs are hard to notice manually, and there are no autotests for vzlist.
* Yes some parts of vzlist is a copy-pasted mess, I am slowly working on untangling it. For example, see my previous cleanup patches (committed back in June 2010):
src/vzlist.c: streamline a few macros
vzlist: put similar print_ functions in a macro
vzlist.c: simplify last_field logic
Morale: sometimes declaring functions as static actually helps!
PS if you see mistakes in this blog post, patches to it are welcome. It's 1am here and I am a bit sleepy.
I was working a bit on vzctl today (my target was bug #1757, which is still a work in progress) and ... I am not sure how, but I ended up declaring most functions in src/vzlist.c as static. I thought it doesn't have any practical value -- I was wrong!
In C, if you declare the function as static, it means its visibility is limited to the translation unit (i.e. a file) in which it is defined. In other words, you can only call/use a static function from another function in the same file.
Now, in vzctl sources vzlist.c is only linked to one binary -- vzlist, and therefore I thought it doesn't make much sense to declare functions as static. Nevertheless I did it (see git commit).
Next thing I got is a set of compiler warnings! OK, all right, let's take a look...
First set of warnings is self-explanatory. See:
vzlist.c:825:14: warning: ‘parse_var’ defined but not used
vzlist.c:1075:14: warning: ‘remove_sp’ defined but not used
vzlist.c:1357:12: warning: ‘get_stop_quota_stats’ defined but not used
Easy! In some ancient time, these functions were used, now the code has changed and no one needs these three, but they were not removed for some reason (probably just forgotten). Solution: remove the dead code (see git commit).
Second set of warnings looks similar:
vzlist.c:400:1: warning: ‘dcachesize_m_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_l_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_b_sort_fn’ defined but not used
vzlist.c:400:1: warning: ‘dcachesize_f_sort_fn’ defined but not used
vzlist.c:411:1: warning: ‘diskinodes_s_sort_fn’ defined but not used
vzlist.c:411:1: warning: ‘diskinodes_h_sort_fn’ defined but not used
Hmm... all these *_sort_fn are sort functions generated by means of a few #define statements, and they are used when vzlist needs to sort its output by some column or parameter (vzlist -s). It is very strange that these are not used, because they should be. Let's take a closer look... zOMG! it's a bug!
Apparently, someone was using copy-paste technique* and forgot to change the names of the functions. The bug is, when you ask vzlist to sort its output to, say, dcachesize failcounter values, it sorts it by dcachesize held values instead, because of the wrong sort function used. Such bugs are hard to notice manually, and there are no autotests for vzlist.
* Yes some parts of vzlist is a copy-pasted mess, I am slowly working on untangling it. For example, see my previous cleanup patches (committed back in June 2010):
src/vzlist.c: streamline a few macros
vzlist: put similar print_ functions in a macro
vzlist.c: simplify last_field logic
Morale: sometimes declaring functions as static actually helps!
PS if you see mistakes in this blog post, patches to it are welcome. It's 1am here and I am a bit sleepy.

Comments
Do you still stand by your opinions above now in 2016?…