Stephen's Work Ramblings

Having spent a while looking at the LCFG client code now it is clear that much of it would benefit from being totally restructured as a set of Object-Oriented classes (probably Moose-based). Making such a big change is beyond the scope of this project but there is still a need to store state in a sensible fashion. Currently the code has a heavy dependence on global variables which are scoped at the module level. In many ways the modules are being used like singleton objects and most of the globals are not accessible from outside of the parent module so it’s not as bad as it could be. The biggest issue with these globals is initialisation, where multiple subroutines need to use a global they all dependent on one of them having initialised the variable first. We are thus in a situation where the order in which the subroutines are called is important. This is bad news for anyone wanting to be able to fully understand the code, it also makes it impossible to test each subroutine in an isolated fashion (i.e. given this input, do I get the right output).

With the move to SL6 we got an upgrade to perl to 5.10, this is still utterly ancient but it does provide a few new handy features. The one I’ve begun using a fair bit is the state function which is used similarly to my. The difference is that these variables will never be reinitialized when a scope is re-entered (whereas my would reinitialize the value every time). This makes it possible to write subroutines which act a bit like Object-Oriented accessors with the values being set to a sensible default value where necessary. I’ve used this to nicely handle the acknowledgement and notification port global variables. Here’s an example:

use feature 'state';

sub AckPort {
    my ($value) = @_;

    # Default: Either from /etc/services or hardwired backup value
    state $ack_port = getservbyname( 'lcfgack', 'udp' ) 
                         // $DEFAULT_PORT_ACK;

    # Allow user to override
    if ( defined $value ) {
        $ack_port = $value;
    }

    return $ack_port;
}

Note that the state feature needs to be specifically enabled to use this approach. On the first call to the AckPort function the $ack_port variable will be initialised. If the getservbyname function returns an undefined value (i.e. the named service was not found) then the default value will be used. If the caller specifies a value then that will override the port number. On subsequent calls the initialisation is not done and the current value will be returned. This provides a public API for getting and setting the port number with simple handling of the default value. There is no issue of needing to know in what sequence of subroutines this method will be called, all functionality is neatly encapsulated. The method is also easily testable. Overall an Object-Oriented approach would be better but this is a good halfway house.

Part of the functionality in the LCFG::Client::Daemon code is to send acknowledgement messages to the LCFG servers whenever a new profile has been applied. The ack is sent via UDP using the SendAck method. The original code to do this took the traditional C-style approach:

  return ::TransientError("can't open UDP socket",$!)
    unless (socket(ACKSOCK,PF_INET,SOCK_DGRAM,$proto));

  return ::TransientError("can't bind UDP socket",$!)
    unless (bind(ACKSOCK,sockaddr_in(0,INADDR_ANY)));
  
  my $addr = inet_aton($name);
  return ::DataError("can't determine host address: $name") unless ($addr);
  
  my $res = send(ACKSOCK,$msg,0,sockaddr_in($aport,$addr));
  return ::TransientError("can't send notification: $name",$!)
    unless ($res == length($msg));

with a smattering of weirdness and unless thrown in for good measure. Things have moved on a bit since the days when this was the recommended approach. There is now a very handy suite of modules in the IO::Socket namespace which can handle the dirty work for us. The replacement code looks like this:

   my $port = AckPort();

    my $socket = IO::Socket::INET->new (
        PeerAddr   => $server,
        PeerPort   => $port,
        Proto      => 'udp',
    ) or return LCFG::Client::TransientError(
             "can't connect to $server:$port", $! );

    my $res = $socket->send($message);

    $socket->close();

    if ( $res != length $message ) {
        return LCFG::Client::TransientError(
                  "can't send notification: $server", $! );
    }

That is, without a doubt, much easier to read and maintain. We are now relying on someone else to do the socket handling but that’s fine as this is a core Perl module which should be totally reliable.

Having finished the tidying of the LCFG::Client::Build module I have now moved onto LCFG::Client::Daemon. The first thing which caught my eye was the handling of the state tables. These state tables are used to control how the daemon handles notifications from the server, timeouts and signals from the LCFG client component. I pretty much totally rewrote the MakeTable function so that it processed the input text and built the data structures for the tables in a much cleaner and more comprehensible manner. As with previous changes, my first step was to write some tests which checked the old function then ran them again with the new code to ensure I had not altered the API. I also introduced a new NextStateInTable function which contained code which was previously duplicated inside NextState. Finally I introduced an InitStateTables function which is called from within ServerLoop which hides the initialisation of the global variables used to hold the state tables. This means we now have a much cleaner API for handling all the state transitions based around smaller, testable functions.

The LCFG::Client::Build module is the largest part of the LCFG client code. It weighs in at 1800 lines which is nearly 50% of all the code in the project. It contains a lot of functionality related to processing the data from the XML profile into the format stored in the local DB file and triggering components to reconfigure as necessary. Improving this code was always going to be a big task but at least once this module is done the remainder will seem easy.

The main changes which stand out are, like with LCFG::Client, related to noticing repeated coding of the same functionality. The first larger change came from noticing that in many places the value of an attribute (for example, the LCFG resource value) are decoded using the HTML::Entities module but only for LCFG profile version 1.1 and newer. Now we probably haven’t supported anything older than this for a very long time but it occurred to me that rather than just drop the version check it would be better to completely enhance the attribute decoding. So, rather than have calls to HTML::Entities::decode all over the place we now pass the value through a new DecodeAttrValue function which in turn calls a new NeedsDecode function to check if decoding is required. These are both small easily testable functions so I added a few tests along the way. The big benefit here is that if we now ever need to change the encoding/decoding of values and increment the profile version we are already prepared for the necessary code modifications.

The second big change was to improve the code of the InstallDBM function. This had two copies of a complex regular expression used to parse a fully-qualified resource name (e.g. host.component.resource_name) so I moved this code into a new function named ParseResourceName. Again this is now easily reusable and testable whereas before it was buried in the midst of other complex code. This led to some other improvements in how the debugging was done, I noticed there were many calls to KeyType which was just returning a prettified name for the underlying attribute type indicators which are all single characters (in the set [#%=^]). Each debug statement was very similar but handled a slightly different case, these were all merged into a ResourceChangesDebug function. This new function massively improves code readability and also improves efficiency since it only actually does something when the "changes" debug option is enabled. By reworking the debugging it is now possible to use the KeyType function in a totally generic manner. Anything which needs to know about the type of the attribute can work with the named versions rather than the almost-meaningless single character indicators.

There is still a lot more to do on this module to really improve the code standards but much of that might well be beyond the scope of this initial code cleanup project. The XML profile parsing and the DB handling are particularly in need of attention.

The first round of tidying code to satisfy perlcritic was focussed on the central LCFG::Client module which contains code used by all the other modules.

As well as the tidying there were a couple of slightly larger changes. I had spotted that several routines (RPMFile, DBMFile and ProfileFile) were each doing their own mangling of the host FQDN and then doing similar work based on the results. To reduce duplication I introduced a SplitFQDN function which contains an improved version of the hostname splitting functionality (and which can now be used in other places). I then also introduced another new function (named HostFile) which contains all the other duplicated functionality between the 3 functions. Each of the 3 functions are now pretty much reduced to a single line call to HostFile with the relevant parameters set. At the same time as adding these new functions I added tests for them and also the higher-level functions which use them. As each has now been reduced in complexity it is much easier to test them. This gives me a good guarantee that if I have to make changes in the future they will continue to work as expected.

Beyond tidying the code to resolve the worst of the perlcritic issues I also applied a number of changes which come from the lower levels. In particular I removed a lot of unnecessary brackets associated with calls to built-in functions and conditionals. This might seem like a tiny thing but it does reduce the “noise” quite considerably and vastly improves the code readability. I also removed all uses of the unless conditional, this is something which drives me crazy, anything more than an utterly simple condition is very hard to comprehend when used in conjunction with unless. That is one feature I really wish was not in Perl! I’ve seen unless-conditions which are so complicated that only a truth table can fathom out what is going on…

Another code smell which was eradicated was the heavy usage of the default scalar variable ($_). In my opinion there is no place for using this in large code bases outside of situations like code blocks for map and grep. Using it pretty much guarantees that there will be the potential for weird, inexplicable action-at-a-distance side-effects in your code.

One thing I would like to spend more time on at some point is improving the naming of variables. There is frequent use of single-letter variable names ($c, $t, etc) which is mostly meaningless. This might not be a problem in a very short (couple of lines) block where the context is clear but in chunks longer than a screen-full it’s really hard to track the purpose of all the variables. There is also quite regular reuse of variable names within a subroutine which again makes mentally tracking the purpose of each variable very difficult.

The next phase of the project to clean up the LCFG client (goals 3, 4 and 5) is to run everything through the perltidy tool and then fix the code to satisfy the perlcritic code checker down to level 4. Having all the code consistently indented with an automated tool may seem like a trivial thing to do but it makes such a difference to the maintainability of code. I realise that Python coders have been going on about this for years so it’s nothing new… We chose a coding style for the LCFG server refactoring project and I am using the same for the LCFG client. At the time we added a few notes on the LCFG wiki PerlCodingStyle page. I guess I probably ought to upload my .perltidyrc configuration file to that page so that it can be easily reused.

The use of perlcritic to check code is probably slightly more controversial for some people. It checks your code against a set of rules and recommendations originally laid out in Damian Conway’s book Perl Best Practices. If you don’t like some of those rules you are going to find it very annoying. We’ve found that aiming to satisfy levels 4 and 5 (the most critical issues) results in a vast improvement in code quality. Below that you very rapidly get into a lot of tedious transformations not all of which give you any great benefit. Knowing when to just ignore the moans of the tool is a very useful skill.

The work to cleanup rdxprof is now pretty much finished. All the functionality has been moved out into the LCFG::Client module so that all that happens in the rdxprof code is 3 simple calls to subroutines in the core module:

1. SetOptions – Parse the command line parameters and sets LCFG::Client variables
2. Init – Initialises the environment (mostly just ensuring certain directories exist)
3. Run – This does the actual work (either OneShot or ServerLoop)

There is still a small number of dependencies on global variables that would be nice to remove in the future but nothing critical for now.

This concludes goals 1 and 2 on the project plan. The hope was that this would only take one day of work but it ended up needing 2 days. That is due to my not having initially spotted the real degree of peculiarity of the coding style. The rdxprof code was definitely much more complex in terms of how it approached the “structure” of the entire program than anything I had encountered in the LCFG server code refactoring project. Hopefully now that particular intricate unpicking job is complete the rest will be more straightforward.

The refactoring of the LCFG client has been continuing at good pace. I have now managed to move all the subroutines from the rdxprof script into the LCFG::Client module. This means that it is now possible to add unit tests for the functionality in these subs and I spent a bit of time yesterday adding the first simple tests. There are a lot more to go but it’s a good start. Adding the tests really helped me load more of the code into my brain so there are more benefits than just having testable code.

The big job for yesterday was really improving the sanity of the global variables. Some of the module code relied on the fact that it was being called from rdxprof to be able to access global variables declared in that script. Thus those modules wouldn’t work properly when loaded individually. In one case (the $root variable) it was declared as a global in two places and used as a local variable in many subroutines when passed in as an argument, that’s just a recipe for utter confusion. I’ve now removed one global but there is clearly a need to improve the situation further.

I also moved all the uses of values which are hardwired at build-time using cpp-style macros (e.g. @FOO@) into readonly variable declarations at the top of the modules. This makes it much more obvious which hardwired strings are required by each module. This is a first step towards replacing this approach with a configuration module (e.g. LCFG::Client::Config) which is how we handled the situation for the LCFG server.

The time has come to start work on refactoring the code base of the LCFG client. This has been overdue for a while now as the current state of the code is preventing us from doing much in the way of new developments for fear of breaking something important. The aim is to tidy and clean the code to bring it up to modern Perl standards and generally make it much more readable and maintainable. The aim is to avoid altering the functionality if at all possible although a number of small bug fixes will be tackled if time allows. The full project plan is available for reading on the devproj site. This project incorporates many of the lessons we learnt when we refactored the LCFG server code last year, again see the devproj site for details.

I made an initial start on the project today. As with all refactoring the best first move is to ensure you have some tests in place. In this case I just added simple compilation tests for each of the 5 Perl modules involved. Interestingly this immediately flagged up a genuine bug which existed in the code, this was related to the use of subroutine prototypes. Now anyone who has a reasonable amount of experience with Perl programming will tell you that subroutine protoypes are evil, full of gotchas and rarely do what you expect. One of the tasks in the plan is to rid the code of them entirely but that’s not for today. Thankfully this was a simple error where the prototype stated that 4 scalars were required when, in actual fact, only 3 were needed (and only 3 were provided when the subroutine was called). I’m surprised the code actually worked at all with that bug, this shows how useful even simple testing can be for improving code quality.

The whole code base is basically 5 Perl modules and a script which uses them all. An interesting strategy was taken with the module loading, all subroutines from the modules were imported into the “main” namespace of the script (which is effectively global) and then all calls to them anywhere in the code base were referred to the version in that namespace. So, all subroutine calls were done with unqualified, short names, I guess this makes it quick to hack out but coming at the code without a huge amount of prior knowledge it is almost impossible to quickly reckon the source location for each subroutine. So, my second step was to work through all the code and replace the calls with fully-qualified names. To make it doubly clear that the old way wasn’t readable or maintainable I also ripped out (the now unnecessary) support for exporting subroutines into another namespace and ensured that when these modules are loaded there is no attempt to import anything.

This sort of change should be zero impact, right? Turns out, not entirely, nothing is ever simple… I had to shuffle a few subroutines out of the script into the helper modules, in turn that meant fixing a few references to global variables. This in turn required passing another parameter to a couple of subroutines which meant hacking out a few evil subroutine prototypes. I think that shows up a few code smells which will have to be tackled very soon.

Before I can really get stuck in though a few more tests are going to be necessary. At the very least there is going to have to be a test of the client’s ability to download an XML profile and convert it into the locally stored file format. At this stage I don’t know enough about the code to create tests for each subroutine so a large-scale test of functionality is the only option. Without that test it won’t be safe to make any bigger code changes.