Date: Sun, 27 Jun 2010 00:23:34 +0200 From: Karsten Behrmann <BearPerson@gmx.net> To: freebsd-sysinstall@freebsd.org Subject: Some design suggestions Message-ID: <3B0E67D1-95E7-421C-A506-744C86BC1E6C@gmx.net>
next in thread | raw e-mail | index | archive | help
Hey all, I pondered a bit about what code problems make sysinstall code hard to read/maintain, and ended up with the following suggestions: [TL;DR: Sysinstall is old and grown, and we should make some adjustments to modules/headers/functions to fit the bigger size] ----- Module API ----- [TL;DR: Use consistent names and prototypes for all public functions that do the same type of thing] As far as I'm aware, there are only a handful of abstract operations that sysinstall needs to perform on each "module" (disk, networking, distribution set, that kind of thing): * configure - Bring up a GUI to let the user choose how to set stuff up * commit - Actually perform configured changes (e.g. write a conf file) * reset - Return configuration to unset/defaults * setup [optional] - bring up inside install environment (e.g. network) * teardown [optional, probably implied by reset] - undo setup I suggest we try and be consistent in naming and typing the functions that do these things, so that functions calling these can be obvious and regular. Maybe configFoo, commitFoo, resetFoo, setupFoo, teardownFoo? I suggest having each typed as int configFoo(void) where appropriate, with the following return codes: * 0 - success * 1 - unspecified error (to catch returns of a && b) * 2 - user initiated abort (by pressing cancel or something) * 3 - operation failed (no disk found, I/O error, ...) The idea is that sometimes we may want to behave differently when the user cancelled (e.g. not retry). For simplicity, we should try to use the same return code convention in module-internal code, when appropriate. Note that I do not include a dialogMenuItem* in the prototype, which brings us to... ----- avoid libdialog in API ----- [TL;DR: libdialog return codes suck. Don't use them. Don't use libdialog arguments unnecessarily either] Making things like diskPartitionEditor() (essentially configPartitions) usable directly as a menu function was useful in the past, as it meant less code to write to add new dialogs. However, I do not really want to worry about libdialog API when I'm working on specific sysinstall code. I really don't like picking my arguments out of self->aux (self being the dialogMenuItem* we get as argument now), whoever calls me ought to do that. And I do not like DITEM_FAILURE as a bool - we have too many places where we forget to slap DITEM_STATUS() on a return code before we compare it with DITEM_SUCCESS/FAILURE. This does mean we need more boilerplate code in some places, but I think in a project this size, some boilerplate is acceptable, and judicious use of macros can help keep it under control. (menus could be done with a wrapper function that picks the real function to call from item->aux, although that means casting a function pointer to long and back, which may not be such a good idea) ----- use more static data ----- [TL;DR: Don't use environment variables to store config info, use static globals inside the modules instead] I do not think much of our current method of storing state: variable_set2, variable_get, etc. Environment variables seem somewhat clunky, and boiling everything down into strings seems prone to typoes and other problems. I would much rather have state inside static variables of the .c file that works with that bit of state. This would also make cross-module dependencies more obvious, as modules would be calling functions / accessing variables that clearly belong to some module, instead of accessing variable_get which does not clearly document who is responsible for the value. It also makes it more obvious which variables make up some module's state, as they would all be collected in a big block near the top of the source file. However, before we can do this, we need to investigate if sysinstall ever actually spawns a program that cares about those environment variables. We also need to implement saving/loading of variables in each module, if we want to remain compatible with old sysinstall.vars files. However, saving/loading should be an afterthought, not the main design perspective of the whole sysinstall application, IMHO. ----- fine-grained headers ----- [TL;DR: Don't use one big sysinstall.h, give each module its own header and include wisely] I believe we are past the point where our big monolithical sysinstall.h is appropriate. I like to be able to check the #include lines at the top of a file to see who this file will be talking to, I cannot do that now. Specifically, I would like to reduce #include of system headers inside sysinstall.h as much as possible, and I would like to give every module its own header file. The PartType enum, for instance, really belongs in a label.h - if you need it from some other module for some reason, it should be obvious that you need disks stuff by #include'ing label.h. (In this case, nobody else actually needs this definition, so why pollute the global namespace?) Another example, SwapChunk is mostly used in install.c, but also referenced in label.c - we should decide who it really belongs to, and stick it there. Or NCPus is dangerously misnamed - it is never actually initialized to the number of CPUs on the machine... This would be more obvious if one module "owned" the variable and was responsible for initializing it at some point. 900 lines to a header are definitely somewhat excessive, anyway. ----- Summary ----- I have some suggestions how the readability of sysinstall code could be improved, through some large policy/style changes. Of course, opinions differ, and this won't be implemented overnight anyway. This is how I personally think we should attack things, and I think if we do, things will become significantly easier to understand. No actual functionality would change, no new features come in, just some general code janitor work. If you think differently on some points, do say so - maybe we can get some sort of productive discussion going. Whatever we end up at, it should be a start towards a more maintainable sysinstall. So Far, Karsten "BearPerson/BearMan" Behrmann
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3B0E67D1-95E7-421C-A506-744C86BC1E6C>