LCFG Client Refactor: Initial tidying done

April 25, 2013

A quick dash through the code in the LCFG::Client::Fetch module (which is relatively small and fairly straightforward) means that all the LCFG client code has been checked using perlcritic and improved where necessary/possible. This completes the work for stages 4 and 5 of the project plan.

Some of the work involved in this stage has been rather more complex than anticipated. Mostly that was not related directly to resolving issues highlighted by perlcritic. In the main it was because whilst investigating the issues raised I spotted other, big problems with sections of code that I felt needed to be resolved. Those could have been kept separate and done as an additional stage in the project plan but I thought it was better to just do them. In particular, I have made large improvements to the sending and receiving of UDP messages for notifications and acknowledgements. I’ve also improved the logic involved with handling the “secure mode” and split out lots of sections of code into smaller, more easily testable, chunks.

This takes the effort expended up to about 7 days. That’s about 1 day over what I had expected but that was accounted for in stages 1 and 2, the gap between predicted and actual effort requirements has not worsened.


LCFG Client Refactor: Storing state

April 24, 2013

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.


LCFG Client Refactor: Sending acks

April 24, 2013

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.


LCFG Client Refactor: Daemon state tables

April 17, 2013

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.


LCFG Client Refactor: tidying LCFG::Client::Build

April 17, 2013

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.


LCFG Client Refactor: tidying LCFG::Client

April 8, 2013

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.


LCFG Client Refactor: perltidy and perlcritic

April 5, 2013

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.


LCFG Client Refactor: rdxprof finished

April 5, 2013

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.


LCFG Client Refactor: rdxprof cleanup

April 2, 2013

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.