Coding style on Quattor modules

Introduction

All major software projects have a set of guidelines for their code. It helps people to understand each other's code, and even your own code a few months (or days!) after you wrote it for the first time.

There are lots of documents motivating this, feel free to review them. ​In short:

• I want to read your code.
• I want you to read my code.
• I want to fix your bugs.
• I want you to fix my bugs.

Further reading

• ​Perl coding style guide
• ​Documentation/CodingStyle and beyond, by Greg Kroah-Hartman

The basics

Indentation

Use 4 white spaces. No tabs, no 8 white spaces, no funny things. Fix your editor's configuration, or use a decent one. There are plenty of them.

Capitalisation

Use lowercase for local variables, uppercase for constants.

We have no convention for function names, and we have functionNamesLikeThis (whichIHate), FunctionNamesLikeThis (WhichIDontLikeEither) or function_names_like_this_one (which I prefer). Do we need such a convention?

Use meaningful names for globals, short names for locals

i is a perfectly valid identifier for a loop variable. However, you deserve nasty punishment if you use it as a function name.

On the opposite, this_is_a_temporary_counter is plain bad for a loop variable.

So, some good examples:

 sub write_user_credentials
 {
     # ...
 }

 for my $i (0..10) {
     # ...
 }

And very bad examples:

 sub wuc
 {
     # WTH???
 }

 for my $counter_from_0_to_10_included (0..10) {
     # Excuse me???
 }

Be modular

Really, it matters. Have you ever tried to understand a module which is 700 lines long? Did you ever try to understand what's the purpose of a variable that was defined 5 screens ago? Or the meaning of the 30th variable on this block? Or the purpose of that line that starts and finishes beyond the screen?

Here are the classic metrics for modularity:

• No lines longer than 80 columns.
  • Split or rearrange longer strings
  • Split longer statements
• If you have more than 3 levels of indentation, split your block
• If your function goes beyond the screen, split it
  • And don't try to reduce the font. ;)
• If you have more than 7 local variables, split your function.
  • Sometimes it's OK to have 10, but if you have 15 your code is a problem.

Don't use magic numbers

The constant pragma will give you meaningful names for any values other than 0 and 1 you need. They'll help you to understand why you chose such values on the past.

Good example:

 use constant PI => 3.141592;

 my $circle_area = $radio * PI * PI;

And the bad example

 # Oops! I missed a decimal somewhere!
 my $circle_area = $radio * 3.14159 * 3.141592;

Comments

When in doubt, don't use them.

Comment all Pan data structures, or at least provide a link to their full description. Try to use the new annotation syntax, which will be (some day) imported by Doxygen, Javadoc or something similar.

Comment the purpose of each file, probably using POD syntax. Write a comment before the beginning of each function, telling what it does and how to use it. But don't annoy the reader with the internals.

Don't comment function bodies. If your code is so complex that it needs further explanations, you should probably split it in several functions and comment those functions. Of course, sometimes you have to work around some broken API, or some corner case. In such case, please comment why you are doing it, but not how. And don't comment the obvious.

Bad examples:

 ############################################################
 #
 # Increment by 1
 #
 ############################################################
 $i++;

 sub do_something
 {
    my @args = @_;
 
    # I do foo and bar here
    ...
    ...
    ...
    # And now, let's do bar again, but with a small difference
    ...
    ...
 }

These are best done like this:

 $i++;

 # Performs task foo...
 sub foo {...}
 # Performs task bar...
 sub bar {...}
 # Performs task baz, which is quite similar to bar
 sub baz {...}

 # Performs a set of tasks, returning blah blah with arguments
 # $arg1:
 # $arg2:
 sub do_something
 {
     my @args = @_;
     foo (@args);
     bar (@args);
     baz (@args);
 }

Finally, don't put any credits on your comments, excepting on file headers.

Don't use vars

This pragma has been deprecated since Perl 5.6, and that's a long time ago. Instead, use the our declaration for package-wide variables:

Good:

 our @ISA = ...;

Bad:

 use vars qw (@ISA);
 @ISA = ...;

Curly bracket position

Follow Kernighan-Ritchie's convention: open curly brackets on the same line as the sentence they belong to and close them on a line for their own, excepting when it's an else or a do-while block:

 if ($foo) {
     ...
 }

 while ($bar) {
     ...
 }

 if ($foo) {
     ...
 } else {
     ...
 }

 do {
     ...
 } while ($bar);    

This way it is perfectly clear where each block starts, finishes and continues.

The only exception to this are the curly brackets that open a function. They should be on a different line, and have nothing else on the same line:

 sub foo
 {
     my @args = @_;
 }

The reason for this is that decent editors (f.i, Emacs) are aware that such curly braces mean something special, and allow you to move to the beginning and the end of a function with a single key stroke. This is a great help for navigating code.

Parenthesis

Use them a lot. When in doubt, use them. Especially:

• Always use parenthesis on function calls
• Always use them on function calls even when there are no arguments

The reason is that:

 foo();

Is easier to understand than:

 foo;

Prefer methods over plain functions

Especially when writing components, you want to log unexpected things. It's free to have a $self object, and let it log.

Only use plain functions if you want them exported to some other module.

The Quattor library (AKA perl-CAF and perl-LC)

Running commands

Do not use backticks or system, nor use open for pipes. The CAF::Process module has all you need, and it won't spawn new subshells, which is much safer.

Another advantage of the CAF::Process module is that your command line will be automatically logged at verbose level, which saves you snippets like this:

 $self->debug (5, "Going to run ls -l");
 system ("ls", "-l");

This will become inconsistent with different uses. Instead, do:

 # Or any reporter object.
 my $proc = CAF::Proces->new (["ls", "-l"], log => $self);
 $proc->run();
 # One-line version:
 CAF::Process->new (["ls", "-l"], log => $self)->run();

The log option is any CAF::Logger object, for instance the component you are writing.

Confidential information on the command line

Sometimes you pass confidential data to your commands. For instance, an encrypted password to usermod. In this cases, you don't want your command logged. Just don't pass any log argument to CAF::Process::new:

 my $proc = CAF::Process->new (["ls", "-l"]);
 $proc->run();

Examples

The following are extracted from CAF::Process man page, and assume you don't want to log the commands.

Piping to a process' stdin

Create the contents to be piped:

 my $contents = "Hello, world";

Create the command, specifying $contents as the input, and "execute" it:

my $proc = CAF::Process->new (["cat", "-"], stdin => $contents);
$proc->execute();

Piping in and out

Suppose we want a bi-directional pipe: we provide the command's stdin, and need to get its output and error:

 my ($stdin, $stdout, $stderr) = ("Hello, world", undef, undef);
 my $proc = CAF::Process->new (["cat", "-"], stdin => $stdin,
                                stdout => \$stdout
                                stderr => \$stderr);
 $proc->execute();

And we'll have the command's standard output and error on $stdout and $stderr.

Emulating backticks to get a command’s output

Create the command:

 my $proc = CAF::Process->new (["ls", "-lh"]);

And get the output:

 my $output = $proc->output();

File handling

Writing to files is not as simple as one could think: there are risks that you should be aware of. For instance, the following code is an example of what shouldn't be done:

 open (FH, ">/tmp/foo");

If /tmp/foo already exists and is a symbolic link to /etc/shadow, you just lost all accounts on your system.

Writing to a file

Don't open files directly. Use the CAF::FileWriter module to do the work for you:

 my $fh = CAF::FileWriter->open ("/my/file", log => $self);
 print $fh "Hello, world!\n";
 $fh->close();

This module will perform all security checks for you, and will log what files you open for writing, so that your configuration is easier to debug.

You can specify the permissions you want at instantiation, too:

 my $fh = CAF::FileWriter->open ("/my/path", mode => 0400, log => $self );
 print $fh "Hello, world!\n";
 $fh->close();

If you find any errors and have to discard your contents, just call the cancel method, and the existing file will be preserved:

 my $fh = CAF::FileWriter->open ("/my/path", mode => 0400, log => $self);
 print $fh "Hello, world!\n";
 if ($error) {
     $fh->cancel();
 }
 $fh->close();

Finally, if the contents you generate are the same as on the original file, nothing will be done, which will keep timestamps, among other properties.

The man page has all the details and more examples.

Temporary files

Don't use them. If you don't use File::Temp, you'll use predictable filenames, and that's just bad. Then, most implementations make temporary files world readable, and you usually don't want that. If you need temporary storage for some text, use an array, IO::String, in-memory files, a CAF::FileWriter or anything like that.

So you want to run a command which needs a file name as an argument, right? Easy. Just pipe to that command, as shown above. And pass /dev/fd/0 as the file name.

Finally, if all these options are not good enough (what are you trying to do?), use File::Temp::tmpfile, which will provide you an anonymous file handle. But please, use this only if you are convinced there is no other way to keep your temporary data.

Modifying an existing file

Sometimes you just want to add a couple of lines to an existing file, without destroying its current contents. Or to remove them. The module CAF::FileEditor is just for that. It is a subclass of CAF::FileWriter, and when you call the string_ref method you'll get a reference to the file's contents so that you can freely update them.

For instance, you only want to change the first "a" for a b on the first line of a file:

 my $fh = CAF::FileEditor->open ("/foo/bar",
                                 log => $self);
 my $ref_to_contents = $fh->string_ref;
 $$ref_to_contents =~ s{a}{b};
 $fh->close();

If you want to add text at the beginning of the file, the head_print method is what you are looking for.

Other tasks with files

When in doubt, use LC:: modules and functions, over standard ones. For instance, use LC::File::copy, LC::File::move instead of File::Copy and friends.

LC:: functions perform more security checks and are less prone to symlink attacks.

Input handling

Make your code run in tainted mode.

This is, don't trust any inputs, even if they are from the profile. During the workshop, Nick proposed to sign profiles (it was proposed 2 years ago, too). While this gets (or not) implemented, profiles are not to be trusted. Sanitise everything just after reading it, and that way you'll be able to use it freely.

Also, when you print files that will be sourced by shell scripts, be sure to print all values between single quotes. This is true for almost every file you have under /etc/sysconfig.

Quality of messages

Don't annoy the user

The main principle is not to annoy the user. Use info and ok methods only for very important messages. A small description of the tasks you are going to perform, if they are complex enough.

Unless he asks to

Provide a detailed trace of any task you perform with verbose messages. Trace DNS queries, URLs being retrieved, services you have to enable or disable...

But you don't need to log files you open or commands you run. The appropriate CAF objects will do so for you.

Debugging output

Don't print user-relevant messages with debug. This should be used only for developer-relevant stuff, such as tracking temporary contents or so.

We don't have any convention for debug levels, is it needed?

Use error only for fatal errors

Use the error method only if this error will make the entire component fail. If you can handle it, or the fail is not really important for the component's results, use info instead. If failing to download a file is OK because you can work around it, or you know you may have no rights to write on AFS, but that's OK, use an info message.

Otherwise, the component will be re-run with no need, and will keep failing and triggering alarms on the user's monitoring system.

Conclusions

Code quality matters. It will reduce bugs, and will make everybody's life easier.

All these conventions can be improved, so feedback will be appreciated. Especially, the CAF library can be extended with whatever task we repeat over and over (f.i, do we need any especial code to do DNS queries or download files from the Internet?).