On Thu, 20 Jul 2017 00:31:25 +0800, Matt Johnston wrote: > I'm less keen on this options patch series. I don't really > think they add much clarity, and any widespread change is > going to cause extra work for people with their own forks. > (Yes the move to localoptions.h is also disruptive, but I > think it has a clear benefit). > It's pretty clear already which options are expected to be > overridden - they're the ones in default_options.h ! > The line between options in sysoptions.h and > default_options.h is kind of vague anyway, something like > KEX_REKEY_TIMEOUT could really go in either, but I've kept > it in sysoptions.h just to try and avoid too many knobs to > fiddle with. The main reason I went with a DROPBEAR_ prefix > is to avoid namespace clashes - CUSTOM_ doesn't really do > that.
Allow me to push back a bit. This switch to `localoptions.h' is already a flag day, so it would probably be prudent to stuff into that flag day as much irritating cleanup as possible. * Sure, nitpicky refactoring is a bit of bikeshedding, but it can be valuable to spend time figuring out which paint color for the bikeshed is actually the most pleasing to look at every day; the choice of color can be very important to those who haven't yet spent over a decade staring at it. This includes decisions that are small, but worthwhile with respect to consistency; for instance, to me, it seems that the file name should be at least `local_options.h' rather than `localoptions.h' (or, maybe, there should be `defaultoptions.h' rather than `default_options.h', as there is already a `sysoptions.h' rather than a `sys_options.h'). Why `local', anyway? That's kind of an archaic term; I mean, one might as well choose `site', like in the old days. Something like `custom_options.h' or `configured_options.h' or `config_options.h' or `user_options.h' carries a lot more information at first glance. What does `sysoptions.h' tell anyone? How does it help structure the thoughts of people who are working with the source code? Indeed, as you already noted, the reasoning for where an option should go is often so nebulous as to be almost arbitrary; that suggests there is something wrong (or at least useless) in the way that the source code is organized. How can we organize it better? Can organization be based around *technical* reasons? * There are 2 kinds of options: * Fundamental constants in the Dropbear Universe. * Variables that can be set by the user. (Hey! what about `variable_options.h.in'?! I kid, I kid... ... but I'm kind of not kidding...) That's starting to look like a hard-nosed, conceptual, technical basis for where certain macros should be defined: Can reasonable people disagree on the value of this macro? Yes! Then it belongs in `config_options.h.in'! (or whatever) No! Then it belongs elsewhere! (and shouldn't be in an `#ifndef' block) Can reasonable people disagree on the value of `KEX_REKEY_TIMEOUT'? Yes! Then it belongs in `user_options.h.in'! (or whatever) Can reasonable people disagree on the value of `DROPBEAR_VERSION'? No! This is the Dropbear project, and if you change this to something custom, then you've effectively forked the project. Thus, it belongs in `constant_options.h' and shouldn't be in an `#ifndef' block. Can reasonable people disagree on the value of `MD5_HASH_SIZE'? No! Blasphemy! The Gods of Crypto handed that one down from their glissening Tower of Ivory, when hackers were hackers and Dennis Ritchie still walked amongst us. Thus, it belongs in... WHERE? Actually, where should `MD5_HASH_SIZE' go, anyway? You'll notice that it's only used in one file: $ git grep -l MD5_HASH_SIZE | grep -v sysoptions.h signkey.c So, why isn't `MD5_HASH_SIZE' just defined in that one file? Indeed, if it were defined in that file, then there probably wouldn't even need to be a qualifying pseudo-namespace, like `DROPBEAR_'. As you can see, some kind of purposeful organizational structure is already beginning to emerge. * Speaking of the qualifying prefix `DROPBEAR_', note that without this refactoring patch series, there are still quite a few option macros that are not yet qualified: $ clean() { awk '{print $2}' | sort -u; } $ filter_options() { git grep -P '^#define\s+(?!DROPBEAR_)' "$1" | clean; } $ $ filter_options default_options.h.in DEFAULT_IDLE_TIMEOUT DEFAULT_KEEPALIVE DEFAULT_KEEPALIVE_LIMIT DEFAULT_PATH DEFAULT_RECV_WINDOW DO_HOST_LOOKUP DO_MOTD DSS_PRIV_FILENAME ECDSA_PRIV_FILENAME INETD_MODE LOG_COMMANDS MAX_AUTH_TRIES MAX_UNAUTH_CLIENTS MAX_UNAUTH_PER_IP MOTD_FILENAME NON_INETD_MODE RECV_MAX_PAYLOAD_LEN RSA_PRIV_FILENAME SFTPSERVER_PATH TRANS_MAX_PAYLOAD_LEN XAUTH_COMMAND $ $ $ filter_options sysoptions.h AUTH_TIMEOUT ENABLE_CONNECT_UNIX ENV_SIZE IS_DROPBEAR_CLIENT IS_DROPBEAR_SERVER KEXHASHBUF_MAX_INTS KEX_REKEY_DATA KEX_REKEY_TIMEOUT LOCAL_IDENT LTM_DESC MAX_BANNER_LINES MAX_BANNER_SIZE MAX_CHANNELS MAX_CMD_LEN MAX_ECC_SIZE MAX_HASH_SIZE MAX_HOSTKEYS MAX_HOST_LEN MAX_IP_LEN MAX_IV_LEN MAX_KEY_LEN MAX_LISTEN_ADDR MAX_MAC_LEN MAX_NAME_LEN MAX_PRIVKEY_SIZE MAX_PROPOSED_ALGO MAX_PUBKEY_SIZE MAX_RECV_WINDOW MAX_STRING_LEN MAX_TERM_LEN MD5_HASH_SIZE MIN_DSS_KEYLEN MIN_PACKET_LEN MIN_RSA_KEYLEN PROGNAME RECV_MAX_PACKET_LEN RECV_WINDOWEXTEND SHA1_HASH_SIZE TRANS_MAX_WINDOW TRANS_MAX_WIN_INCR _PATH_CP _PATH_TTY Surely, for the sake of consistency, and to avoid clashes, these macros should also be prefixed with `DROPBEAR_' (or moved to a more relevant file), especially now that there is already going to be a flag day. And, if they are going to be altered, then why not also institute some kind of differentiator that indicates whether the macro in question represents either a Dropbear-defined constant or a user-defined variable? This may seem trivial, but it might also be very useful to be able to see right in the very code, at the very point of use, that a particular value represents input from the user; it would be a more explicit way to identify exactly where the user interfaces with the Dropbear source code. This differentiator doesn't have to be `CUSTOM_'; it could instead be `DROPBEAR_USER_'. Certainly, it might help people who fork the code identify what exactly is considered acceptable to manipulate without the consensus of the wider userbase; if a user manipulates something that is not explicitly presented as being user-definable, then that user is acknowledging the risk that he might have to deal with source-code conflicts in the future. * Even those users who have forked the code will likely have major difficulties only with one file (`options.h'), and with one aspect of the source code (how user-defined options are specified). This will already be the case, regardless of the refactoring; it hardly counts against refactoring in general. * Even with heavy refactoring, it seems like it should be possible to help users convert to the new system in a way that would be seamless for most cases. Rather than using `options.h' for the new system, the file to use should instead be, say, `all_options.h'; not only would this be named consistently with the related files (`default_options.h.in', and, say, `constant_options.h'), but it would allow for keeping around the old (untouched) `options.h' file (and perhaps also the old `sysoptions.h'), so that a user's file manipulations can still occur without conflict. Then, when `make' is run as usual, if the user has not yet supplied a `custom_options.h' (or whatever), a new `custom_options.h' can be created by converting the old options into the new options; perhaps the conversion could be affected by whether any old file has actually been altered. There could also be a warning printed on `stderr', and a 20 second pause in the build process, to warn a user that the old way of doing things is being abandoned, and to explain that a `custom_options.h' has been created automatically, and to point out any discrepancies that might exist due to this conversion process (if there are any worth mentioning). A user who builds Dropbear as part of a larger, automated building process should receive such a notification if the `stderr' output is being logged for later review. In short, this is potentially a great opportunity to clean up the source code, establishing a more solid foundation for future development. Sincerely, Michael Witten