www/cleanup.html

   1 <html><head><title>toybox cleanup</title></head>
   2 <!--#include file="header.html" -->
   3
   4 <h1>Index</h1>
   5
   6 <ul>
   7 <li><a href=#intro>Introduction</a></li>
   8 <li><a href=#advice>Advice</a></li>
   9 <li>Commands:</li>
  10 <ul>
  11 <li><a href=#uuencode>uuencode</a></li>
  12 <li><a href=#uudecode>uudecode</a></li>
  13 <li><a href=#ifconfig>ifconfig</a></li>
  14 <li><a href=#find>find</a></li>
  15 </ul>
  16 </ul>
  17
  18 <hr>
  19
  20 <a name=intro />
  21 <h1>Cleaning up the toybox code.</h1>
  22
  23 <p>Toybox <a href=http://landley.net/notes.html#31-03-2013>hasn't always</a>
  24 taken proper advantage of external contributions</a>.
  25 Lots of people want to help, but their contributions languish out of tree
  26 or in the "pending" directory, awaiting cleanup.</p>
  27
  28 <p>Toybox's design goals require simpler, tighter, and more explicit code
  29 than most other implementations, among other reasons to allow better security
  30 auditing. Writing "another" implementation of standard command line tools
  31 isn't very interesting, they should be _better_ implementations.
  32 Unfortunately, this means existing, working commands often take more effort to
  33 clean up to Toybox's standards than writing a new one from scratch, not
  34 because they don't work, but because we aim for an unusual level of polish.</p>
  35
  36 <p>In hopes of teaching more people how to do this
  37 cleanup work, I've started breaking cleanup changes into smaller chunks and
  38 posting explanations of each change to the mailing list.
  39 Below are indexes of such cleanup series. Each commit and post are meant to
  40 be read together: each description explains what the corresponding patch
  41 was trying to accomplish.</p>
  42
  43 <p>Line/byte totals of completed series are given for scale, but the point
  44 of this work is simplicity and compactness, not size per se.</p>
  45
  46 <p>(Note: mercurial's web viewer doesn't follow renames, so although each
  47 command name links to a commit list with the bulk of the changes, it may
  48 not include the final version of each file moved from the "pending"
  49 directory to its final location. The summaries link the initial and cleaned
  50 versions of each file when giving line counts.)</p>
  51
  52 <hr>
  53
  54 <a name=advice />
  55 <h1>General advice and/or policy.</h1>
  56
  57 <p>The <a href=design.html>design of toybox</a> page and the coding
  58 style section at the start of the <a href=code.html>source code walkthrough</a>
  59 don't cover everything. Here are some
  60 links to mailing list messages that cover various programming topics
  61 not directly related to a specific cleanup series:</p>
  62
  63 <ul>
  64 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000850.html>Error messages and internationalization.</a></li>
  65 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000891.html>Why not "const"?</a> (Exception: global variables
  66 outside of GLOBALS can be static const, to go in rodata instead of data.
  67 This means the pages can be shared between instances.)</li>
  68 <li><a href=http://lkml.indiana.edu/hypermail/linux/kernel/1308.3/03890.html>Why not "bool"?</a> (explanation from Linus Torvalds)</li>
  69 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000893.html>Why not to check in debug code.</a></li>
  70 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001044.html>Relationship between /proc and /sys</a> (/proc isn't obsolete and /sys is an ABI)</li>
  71 <li>"Hiding numbers that are used just once into defines to put them out of
  72 sight does not really help readability."</a> -
  73 <a href=http://lkml.iu.edu/hypermail/linux/kernel/1407.1/00299.html>Pavel
  74 Machek</a></li>
  75 <li>"Infrastructure in search of a user" is a bad thing, so don't put code
  76 in lib/ that doesn't already have at least two users. Don't preemptively
  77 factor stuff out, it's easy enough to rewrite it uin future if it needs
  78 to change. The "extreme programming" fad called this "You Ain't Gonna
  79 Need It" (while inexplicably screaming at cans of Mountain Dew, back in the
  80 90's). Here's <a href=https://lwn.net/Articles/683745/>Linus Torvalds' take</a>.</li>
  81 </ul>
  82
  83 <hr>
  84
  85 <a name="uuencode"><h1><a href=/hg/toybox/log/900/toys/pending/uuencode.c>uuencode</a></h1>
  86
  87 <p>This is an example of cleaning up something most projects would be quite
  88 happy with. The initial submission of uuencode and uudecode was high
  89 quality code, written by a seasoned developer who did an excellent
  90 job, but it was still possible to shrink the result almost by half:</p>
  91
  92 <ul>
  93 <li>old total: <a href=/hg/toybox/file/828/toys/pending/uuencode.c>116 lines (2743
  94 bytes) in 7 functions</a></li>
  95 <li>new total: <a href=/hg/toybox/file/841/toys/posix/uuencode.c>67 lines (1440
  96 bytes) in 1 function</a></li>
  97 </ul>
  98
  99 <ul>
 100 <li>commit: <a href=/hg/toybox/rev/830>830</a>: first pass, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000904.html>part 1</a>,
 101 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000903.html>part 2</a></li>
 102 <li>commit: <a href=/hg/toybox/rev/831>831</a>,
 103 second pass, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000919.html>part 3</a></li>
 104 <li>commit: <a href=/hg/toybox/rev/837>837</a>,
 105 description: fix test suite.</li>
 106 <li>commit: <a href=/hg/toybox/rev/853>853</a>, description: bugfix.</li>
 107 </ul>
 108
 109 <p>Status: COMPLETE</p>
 110
 111 <a name="uudecode"><h1><a href=/hg/toybox/log/900/toys/pending/uudecode.c>uudecode</a></h1>
 112
 113 <p>The uudecode cleanup was my second "explain as I go along" cleanup,
 114 and I tried to do it in smaller stages so it was easier to see what
 115 changed from the diff:</p>
 116
 117 <ul>
 118 <li>old: <a href=/hg/toybox/file/828/toys/pending/uudecode.c>175
 119 lines (4534 bytes) in 9 functions</a></li>
 120 <li>new: <a href=/hg/toybox/file/840/toys/posix/uudecode.c>107 lines
 121 (2300 bytes) in 1 function</a></li>
 122 </ul>
 123
 124 <ul>
 125 <li>commit: <a href=/hg/toybox/rev/833>833</a>,
 126 description: preparatory adjustments to test suite.</li>
 127 <li>commit: <a href=/hg/toybox/rev/835>835</a>,
 128 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-January/001532.html>Redo command line parsing, redo parsing loop.</a></li>
 129 <li>commit: <a href=/hg/toybox/rev/838>838</a>,
 130 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-January/001533.html>Redo b64_1byte, b64_4bytes, and uu_line()</a></li>
 131 <li>commit: <a href=/hg/toybox/rev/839>839</a>,
 132 description: todo</a></li>
 133 <li>commit: <a href=/hg/toybox/rev/840>840</a>,
 134 description: todo (finish, move pending->posix, default y)</a></li>
 135 </ul>
 136
 137 <p>Status: COMPLETE</p>
 138
 139 <a name=ifconfig>
 140 <h1><a href=/hg/toybox/log/tip/toys/pending/ifconfig.c>ifconfig</a></h1>
 141
 142 <p>This series describes a thorough cleanup that took a while to do.</p>
 143
 144 <p>When ifconfig was submitted, it touched a half-dozen files. I glued it
 145 together into a single self-contained file, which needed a lot of
 146 cleanup. The final version is about 1/3 the size of the original.</p>
 147
 148 <ul>
 149 <li>old total: <a href=/hg/toybox/file/841/toys/pending/ifconfig.c>1504 lines (44268 bytes) in 38 functions</a></li>
 150 <li>new total: <a href=/hg/toybox/file/1133/toys/other/ifconfig.c>521 lines (15963 bytes) in 4 functions</a></li>
 151 </ul>
 152
 153 <p>This was the first command I started cleaning up with the intent of
 154 describing the process, and especially the first few entries in this
 155 series describe many of the low hanging fruit techniques used to clean
 156 up a codebase.</p>
 157
 158 <ul>
 159 <li>commit: <a href=/hg/toybox/rev/843>843</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000882.html>Infrastructure in search of a user, code proximity,
 160 unnecessary #defines, shorter code is easier to read.</a></li>
 161 <li>commit: <a href=/hg/toybox/rev/844>844</a>,
 162 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000881.html>Headers, replacing repeated code with loops,
 163 logical operators guaranteed to return 0 or 1, math on string constants,
 164 removing unnecessary variables and tests.</a></li>
 165 <li>commit: <a href=/hg/toybox/rev/852>852</a>,
 166 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000921.html>hg commit numbers, documenting the obvious, ordering
 167 to avoid prototypes, returning void, collate local declarations,
 168 use error_exit(), unnecessary parentheses, inline to remove variables/functions
 169 used only once, using *var instead of var[0], unnecessary typecasts,
 170 xprintf("\n") vs xputc('\n')</a></li>
 171 <li>commit: <a href=/hg/toybox/rev/856>856</a>,
 172 description: one line portability fix from Isaac Dunham</li>
 173 <li>commit: <a href=/hg/toybox/rev/861>861</a>
 174 and <a href=/hg/toybox/rev/863>863</a>,
 175 description:
 176 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000910.html>Help
 177 infrastructure cleanup from Isaac Dunham</a>
 178 (which I mis-applied and then <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000920.html>fixed plus some whitespace changes</a>)</li>
 179
 180 <li>commit: <a href=/hg/toybox/rev/862>862</a>, description:
 181 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-January/001525.html>remove unused headers and function, replace local buffer with toybuf, perror_exit(), avoid unnecessary assignment.</a></li>
 182 <li>commit: <a href=/hg/toybox/rev/864>864</a>, description:
 183 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-January/001526.html>use common linked list functions, inline set_data, add xioctl, clean up error messages, whitespace and comment tweaks, remove NOP return statements</a></li>
 184 <li>commit: <a href=/hg/toybox/rev/866>866</a>, description:
 185 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-January/001528.html>move standalone globals into GLOBALS() block, collate structs into
 186 iface_list. Inline/rewrite/remove field_format, iface_flags_str,
 187 omit_whitespace(), print_iface_flags(), print_media(), get_ifconfig_info(),
 188 and clear_list(). Merge duplicate function
 189 calls. Show get_proc_info() version field can't matter in 2.6 or newer kernel,
 190 and that SIOCGIFMAP has been there since 1994 so doesn't need an #ifdef.
 191 Loop simplification in readconf() and show_iface().</a></li>
 192
 193 <li>commit: <a href=/hg/toybox/rev/869>869</a> and <a href=/hg/toybox/rev/870>870</a>,
 194 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000928.html>869:
 195 reorder to eliminate prototypes, put command_main() at end of file,
 196 870: long repeated variable prefixes, replacing struct+sscanf()+printf with a
 197 loop and a table (from iface_list, get_proc_info(), display_ifconfig()),
 198 use lib/xwrap.c functions to return void, why xstrcpy() fails closed,
 199 (functional comment: why multicast failed, CSLIP obsolecense), not being
 200 _too_ clever.</a></li>
 201 <li>commit: <a href=/hg/toybox/rev/878>878</a> and <a href=/hg/toybox/rev/879>879</a>:
 202 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000946.html>878: add xsocket(), free(NULL) is a safe NOP (posix!).
 203 879: inline three functions and simplify, some simplifications only show up
 204 after repeated inlining</a></li>
 205 <li>commit: <a href=/hg/toybox/rev/883>883</a>,
 206 description: move some common code to lib/ and posix headers to toys.h.</li>
 207 <li>commit: <a href=/hg/toybox/rev/898>898</a>,
 208 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/000974.html>Argument parsing. (Replace ifconfig_main() if/else staircase with a loop over
 209 an array, genericize minus prefix logic, inline a use of set_flags().)</a></li>
 210 <li>commit: <a href=/hg/toybox/rev/905>905</a>,
 211 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/000992.html>remove unnecessary wrapper function, inlining more functions,
 212 relying on the values of constants that don't change across architectures
 213 (binary backwards compatability), more ifconfig_main table work,
 214 man ioctl_list.</a></li>
 215 <li>commit: <a href=/hg/toybox/rev/906>906</a>,
 216 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/000994.html>More ifconfig_main() table work, remove vestigial arguments
 217 to functions, "a frenzy of inlining", slightly better error reporting,
 218 don't reinvent libc functions.</a></li>
 219 <li>commit: <a href=/hg/toybox/rev/907>907</a>,
 220 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/000995.html>inlining show_ip_addr() with a loop and a table, inlining hex_to_binary()
 221 and set_hw_address(), stop validating data we supplied, remove a table
 222 that's overkill (two entries, we don't even loop over it), when you don't need a
 223 NULL terminator for array, remove unnecessary memcpy(offsetof()) with
 224 assignment, trusting -funsigned-char.</a></li>
 225 <li>commit: <a href=/hg/toybox/rev/919>919</a>,
 226 <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001027.html>todo whitespace damage, introduce IFREQ_OFFSZ() and poke() to
 227 ifconfig_main() table logic to fold more if/else parts into the table</a></li>
 228 <li>Status update: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001033.html>Entering the home stretch on ifconfig</a> (and a <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001043.html>note about infiniband</a>)</li>
 229 <li>commit: <a href=/hg/toybox/rev/921>921</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2014-June/003508.html>Inline a couple more functions and make sockfd a global.</li>
 230 <li>commit: <a href=/hg/toybox/rev/957>957</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-July/001121.html>Remove unused socklen and addr_to_len(), cleanup so we can merge get_device_info()/display_ifconfig().</a></li>
 231 <li>commit: <a href=/hg/toybox/rev/958>958</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-July/001131.html>This commit removes struct if_list by unifying get_device_info() and display_ifconfig().</a></li>
 232 <li>commit: <a href=/hg/toybox/rev/1104>1104</a>, description: Merge toynet into toys.h: musl supports it and micromanaging uClibc config options isn't very interesting anymore.</li>
 233 <li>commit: <a href=/hg/toybox/rev/1127>1127</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-November/001464.html>Start tacling ipv6 issues, beginning with display_ifconfig().</a></li>
 234 <li>commit: <a href=/hg/toybox/rev/1128>1128</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-November/001463.html>More ipv6, make struct sockaddr_with_len go away, merge more arguments into the table in main().</a></li>
 235 <li>commit: <a href=/hg/toybox/rev/1132>1132</a>, description: promotion from pending to other</li>
 236 <li>commit: <a href=/hg/toybox/rev/1132>1133</a>, description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-November/001462.html>cleanup help text, remove obsolete/NOP commands</a></li>
 237 </ul>
 238
 239 <p>Status: COMPLETE.</p>
 240
 241 <h1><a href=/hg/toybox/log/tip/toys/pending/find.c>find</a></h1>
 242
 243 <pre>
 244 814 Initial version by Gurang Shastri.
 245 815
 246 816
 247 847 Felix Janda
 248 848 Whitespace (reindent from tabs -> 2 spaces)
 249 849 More cleanup
 250 867 Felix Janda Improve operator processing.
 251 874 Felix Janda
 252 875 Felix Janda
 253 887 Felix Janda fix -mtime
 254 </pre>
 255
 256 <ul>
 257 <li>commit: <a href=/hg/toybox/rev/849>849</a>,
 258 description: <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000895.html>here</a></li>
 259 </ul>
 260
 261 <p>Status: in progress.</p>
 262
 263 <h1><a href=/hg/toybox/log/917/toys/pending/stat.c>stat</a></h1>
 264
 265 <p>A lot of the stat cleanup was done by Felix Janda.</p>
 266
 267 <ul>
 268 <li>commit <a href=/hg/toybox/rev/747>747</a>: initial submission</a></li>
 269 <li>commit <a href=/hg/toybox/rev/810>810</a>: whitespace</li>
 270 <li>commit <a href=/hg/toybox/rev/811>811</a>: description in commit message.</li>
 271 <li>commit <a href=/hg/toybox/rev/871>871</a>: whitespace (reindent from 4 spaces to 2)</li>
 272 <li>commit <a href=/hg/toybox/rev/872>872</a>: Felix Janda - <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000923.html>cleanup</a> (with discussion thread)</li>
 273 <li>commit <a href=/hg/toybox/rev/875>885</a>: Felix Janda - <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-April/000936.html>move permission formatting (777 -> -rwxrwxrwx) from ls to lib so stat can reuse it.</a></li>
 274 <li>commit <a href=/hg/toybox/rev/885>886</a>: Felix Janda - remove unimplemented options and clean up help text</li>
 275 <li>commit <a href=/hg/toybox/rev/910>910</a>: Felix Janda - <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/001013.html>Add support for stating multiple files</a>.</li>
 276 <li>commit <a href=/hg/toybox/rev/911>911</a>: Felix Janda - Separate stat and statfs, give stat_main() a ds[2] array to distinguish FLAG_f vs not cases, and some whitespace changes.</li>
 277 <li>commit <a href=/hg/toybox/rev/912>912</a>: description in commit message (also <a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/001019.html>here</a>)</li>
 278 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-May/001024.html>design pondering</a> (leading to peek() function in lib/)</li>
 279
 280 <li>commit <a href=/hg/toybox/rev/914>914</a>: description in commit message.</li>
 281 <li>commit <a href=/hg/toybox/rev/916>916</a>: description in commit message.</li>
 282 <li>commit: <a href=/hg/toybox/rev/917>917</a>: description in commit message.</li>
 283 <li>commit: <a href=/hg/toybox/rev/918>918</a>,
 284 description: done: move pending to posix, default y, no other changes</a>.</li>
 285 <li><a href=http://lists.landley.net/pipermail/toybox-landley.net/2013-June/001026.html>summary</a></li>
 286 </ul>
 287
 288 <p>Status: COMPLETE.</p>
 289
 290 <!--#include file="footer.html" -->