Some design suggestions

[Karsten Behrmann]

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