Developers of Sympa

[IKEDA Soji <address@concealed>]

Guillaume and all,

On Mon, 07 Oct 2013 20:15:19 +0200
Guillaume Rousse <address@concealed> wrote:

> Le 07/10/2013 17:55, Guillaume Rousse a écrit :
> > Hence my point: to be discussed later, once we have a working code base,
> > on specific use case.
> Here is an alternative proposal, a little more positive regarding 
> current topic.
> 
> 1) select a simple ternal function (for instance 
> Sympa::Tools::File::set_file_rights())
> 2) replace its current error handling logic with exceptions
> 3) find every place in Sympa code where this function is called
> 4) replace every usage of the function to ensure the exception won't 
> propagate further, and the error is properly logged
> 5) add unit tests checking your modifications
> 6) iterate
> 
> This way, you're setting up your error handling bottom-up, rather than 
> top-down. And if ever you have to refactor it, as you missed something, 
> you have unit tests to check regressions.

It is not coding rule.

Anyway, I'll try to find it from actual code.

Here is a playtest, snippets from current code and revised ones
according to top half of your proposal.  As both set_file_rights.pm*
files are perltidy'ed, they may be easily diff'ed.

Sympa::Tools::File::set_file_rights():

- New exception Sympa::Exception::File was defined.

- "do_log; return undef" logic was replaced with throw().

- As a result, this function no longer need returning values,
  even when it succeeded: "return 1" was also removed.

  - It is not necessarily usual.  Many functions may both return
    values and throw exceptions.

Sympa::Upgrade::to_utf8():

- The logic checking value of set_file_rights() had been replaced
  with catch().

- do_log() is exceuted here.  There no longer are cascaded err logs.

  - I added new full_message() method to exception object: Because
    stringified "$e" returns full traceback with multiple lines
    (we had defined such), it's not suitable for logging.

- This case might be "catch-all" case, namely, when any exceptions
  are thrown, eval'ed process can be concidered to fail _completely_.
  If it's the case, isa() and throw() to filter and to re-throw
  exceptions might be omitted:
  
    if (my $e = Sympa::Exception->catch) {
        Sympa::Log::Syslog::do_log( 'err', '%s', $e->full_message );
        next;
    }

  - However, see the next.

Sympa::Configuration::checkfiles_as_root():

- The same replacement were made (in two places).

- On exception revision, "return undef" are always executed again in
  catch-block.
  This fact suggests that exceptions would not handled here but in
  higher level: Bare set_file_rights(), not wrapped with eval-catch
  structure, would be placed here so that exceptions would pass
  through this function.
  (Final revision of checkfiles_as_root() is attached sepalately).

  - On the other hand, in higher level(s), catch-block will catch
    the exception which were thrown by the code not directly
    enclosed by corresponding eval{}.  For example in sympa.pl:

      eval {
          Sympa::Configuration::checkfiles_as_root()
      };
      if (my $e = Sympa::Exception->catch) {
          # Sympa::Exception::File from lower function can be caught.
          ...
      }

    In other words, it is always probable that catch-block will
    catch unexpected exceptions.
    That's why I argued that uncaught exceptions _must_ be
    re-thrown.  If they were not propagated, serious errors and
    bugs can be hidden.

  - Suggestion #1: catch-block must re-throw uncaught exceptions.

    - Take one more step.  In actual code, failure on
      checkfiles_as_root() let sympa.pl terminate immediately.
      Thus, exceptions thrown through this function need not be
      caught: They should simply be escalated to uppermost level,
      Perl itself:

        Sympa::Configuration::checkfiles_as_root(); # will die on failure.

    - Suggestion #2: Never capture (by eval{}) nor catch (by catch())
      any exceptions, _if_ you don't plan to process them.

      - However again, see the next.

Sympa::Tools::Daemon::write_pid():

- The similar replacements were made.

  - Here, Sympa::Lock::lock() and some built-in functions are assumed
    to throw exceptions.

- Despite exceptions thrown by lower levels will pass through
  this function, eval-catch structure is required in this function,
  because of clean-up process.

  - Suggestion #3: If clean-up process is required, capture and catch
    exceptions, and execute clean-up process, then re-throw uncaught
    exceptions.

Conclusion:

Three suggestions about coding rule were discovered.
- Suggestion #1: catch-block must re-throw uncaught exceptions.
- Suggestion #2: Never capture (by eval{}) nor catch (by catch())
  any exceptions, _if_ you don't plan to process them.
- Suggestion #3: If clean-up process is required, capture and catch
  exceptions, and execute clean-up process, then re-throw uncaught
  exceptions.

Code was refactored to suit to exception scheme.
- It improves error recovery: For example now filehandles are
  assured to be closed and unlocked when any errors occurred.
- It improves logging behavior: Cascaded err logs no longer occur,
  and we can always get complete tracebacks.
- However, there is one open question: It is not sure that it
  improves readability and easiness of writing.


Playtest is done.


Regards,

--- Soji

-- 
株式会社 コンバージョン  セキュリティ&OSSソリューション部   池田荘児
〒231-0004　神奈川県横浜市中区元浜町3-21-2 ヘリオス関内ビル7F
e-mail address@concealed  TEL 045-640-3550
http://www.conversion.co.jp/
package Sympa::Tools::File;

#...

=over

=item set_file_rights(%parameters)

Sets owner and/or access rights on a file.

=back

=cut

sub set_file_rights {
    my %param = @_;
    my ( $uid, $gid );

    if ( $param{'user'} ) {
        unless ( $uid = ( getpwnam( $param{'user'} ) )[2] ) {
            Sympa::Log::Syslog::do_log( 'err',
                "User %s can't be found in passwd file",
                $param{'user'} );
            return undef;
        }
    } else {

        # A value of -1 is interpreted by most systems to leave that value
        # unchanged.
        $uid = -1;
    }
    if ( $param{'group'} ) {
        unless ( $gid = ( getgrnam( $param{'group'} ) )[2] ) {
            Sympa::Log::Syslog::do_log( 'err', "Group %s can't be found",
                $param{'group'} );
            return undef;
        }
    } else {

        # A value of -1 is interpreted by most systems to leave that value
        # unchanged.
        $gid = -1;
    }
    unless ( chown( $uid, $gid, $param{'file'} ) ) {
        Sympa::Log::Syslog::do_log( 'err',
            "Can't give ownership of file %s to %s.%s: %s",
            $param{'file'}, $param{'user'}, $param{'group'}, "$!" );
        return undef;
    }
    if ( $param{'mode'} ) {
        unless ( chmod( $param{'mode'}, $param{'file'} ) ) {
            Sympa::Log::Syslog::do_log( 'err',
                "Can't change rights of file %s to %o: %s",
                $param{'file'}, $param{'mode'}, "$!" );
            return undef;
        }
    }
    return 1;
}

#...

package Sympa::Upgrade;

#...

=over

=item to_utf8($files)

Used to encode files to UTF-8. also add X-Attach header field if template
requires it

Parameters:

=over

=item arrayref

A list of filepath/lang pairs

=back

=back

=cut

sub to_utf8 {
    my $files = shift;

    my $with_attachments = qr{ archive.tt2 | digest.tt2 | get_archive.tt2 |
            listmaster_notification.tt2 | message_report.tt2 | moderate.tt2 |
            modindex.tt2 | send_auth.tt2 }x;
    my $total;

    foreach my $pair ( @{$files} ) {
        my ( $file, $lang ) = @$pair;
        unless ( open( TEMPLATE, $file ) ) {
            Sympa::Log::Syslog::do_log( 'err', "Cannot open template %s",
                $file );
            next;
        }

        my $text     = '';
        my $modified = 0;

        ## If filesystem_encoding is set, files are supposed to be encoded 
according to it
        my $charset;
        if ( ( defined $Conf::Conf::Ignored_Conf{'filesystem_encoding'} ) &
            ( $Conf::Conf::Ignored_Conf{'filesystem_encoding'} ne 'utf-8' ) )
        {
            $charset = $Conf::Conf::Ignored_Conf{'filesystem_encoding'};
        } else {
            Sympa::Language::PushLang($lang);
            $charset = Sympa::Language::GetCharset;
            Sympa::Language::PopLang;
        }

        # Add X-Sympa-Attach: headers if required.
        if (   ( $file =~ /mail_tt2/ )
            && ( $file =~ /\/($with_attachments)$/ ) ) {
            while (<TEMPLATE>) {
                $text .= $_;
                if (m/^Content-Type:\s*message\/rfc822/i) {
                    while (<TEMPLATE>) {
                        if (m{^X-Sympa-Attach:}i) {
                            $text .= $_;
                            last;
                        }
                        if (m/^[\r\n]+$/) {
                            $text .= "X-Sympa-Attach: yes\n";
                            $modified = 1;
                            $text .= $_;
                            last;
                        }
                        $text .= $_;
                    }
                }
            }
        } else {
            $text = join( '', <TEMPLATE> );
        }
        close TEMPLATE;

        # Check if template is encoded by UTF-8.
        if ( $text =~ /[^\x20-\x7E]/ ) {
            my $t = $text;
            eval { Encode::decode( 'UTF-8', $t, Encode::FB_CROAK ); };
            if ($@) {
                eval {
                    $t = $text;
                    Encode::from_to( $t, $charset, "UTF-8",
                        Encode::FB_CROAK );
                };
                if ($@) {
                    Sympa::Log::Syslog::do_log( 'err',
                        "Template %s cannot be converted from %s to UTF-8",
                        $charset, $file );
                } else {
                    $text     = $t;
                    $modified = 1;
                }
            }
        }

        next unless $modified;

        my $date = strftime( "%Y.%m.%d-%H.%M.%S", localtime(time) );
        unless ( rename $file, $file . '@' . $date ) {
            Sympa::Log::Syslog::do_log( 'err',
                "Cannot rename old template %s", $file );
            next;
        }
        unless ( open( TEMPLATE, ">$file" ) ) {
            Sympa::Log::Syslog::do_log( 'err', "Cannot open new template %s",
                $file );
            next;
        }
        print TEMPLATE $text;
        close TEMPLATE;
        unless (
            Sympa::Tools::set_file_rights(
                file  => $file,
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
                mode  => 0644,
            )
            ) {
            Sympa::Log::Syslog::do_log( 'err', 'Unable to set rights on %s',
                $file );
            next;
        }
        Sympa::Log::Syslog::do_log( 'notice',
            'Modified file %s ; original file kept as %s',
            $file, $file . '@' . $date );

        $total++;
    }

    return $total;
}

#...

package Sympa::Configuration;

#...

=over

=item checkfiles_as_root()

Check required files and create them if required

=back

=cut

sub checkfiles_as_root {

    my $config_err = 0;

    ## Check aliases file
    unless ( -f $Conf{'sendmail_aliases'}
        || ( $Conf{'sendmail_aliases'} =~ /^none$/i ) ) {
        unless ( open ALIASES, ">$Conf{'sendmail_aliases'}" ) {
            Sympa::Log::Syslog::do_log(
                'err',
                "Failed to create aliases file %s",
                $Conf{'sendmail_aliases'}
            );

# printf STDERR "Failed to create aliases file %s", $Conf{'sendmail_aliases'};
            return undef;
        }

        print ALIASES
            "## This aliases file is dedicated to Sympa Mailing List 
Manager\n";
        print ALIASES
            "## You should edit your sendmail.mc or sendmail.cf file to 
declare it\n";
        close ALIASES;
        Sympa::Log::Syslog::do_log(
            'notice',
            "Created missing file %s",
            $Conf{'sendmail_aliases'}
        );
        unless (
            Sympa::Tools::File::set_file_rights(
                file  => $Conf{'sendmail_aliases'},
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
                mode  => 0644,
            )
            ) {
            Sympa::Log::Syslog::do_log(
                'err',
                'Unable to set rights on %s',
                $Conf{'sendmail_aliases'}
            );
            return undef;
        }
    }

    foreach my $robot ( keys %{ $Conf{'robots'} } ) {

        # create static content directory
        my $dir = get_robot_conf( $robot, 'static_content_path' );
        if ( $dir ne '' && !-d $dir ) {
            unless ( mkdir( $dir, 0775 ) ) {
                Sympa::Log::Syslog::do_log( 'err',
                    'Unable to create directory %s: %s',
                    $dir, $ERRNO );
                printf STDERR 'Unable to create directory %s: %s', $dir,
                    $ERRNO;
                $config_err++;
            }

            unless (
                Sympa::Tools::File::set_file_rights(
                    file  => $dir,
                    user  => Sympa::Constants::USER,
                    group => Sympa::Constants::GROUP,
                )
                ) {
                Sympa::Log::Syslog::do_log( 'err',
                    'Unable to set rights on %s', $dir );
                return undef;
            }
        }
    }

    return 1;
}

#...

package Sympa::Tools::Daemon;

#...

=over

=item write_pid(%parameters)

Parameters:

=over

=item C<directory> => path

The PID file directory.

=item C<daemon> => string

The daemon name.

=item C<pid> => string

The daemon PID.

=item C<multiple_process> => boolean

FIXME

=item C<method> => string

The PID file locking method.

=item C<user> => string

The PID file user.

=item C<group> => string

The PID file group.

=back

=back

=cut

sub write_pid {
    my (%params) = @_;

    ## Create piddir
    mkdir( $params{directory}, 0755 ) unless ( -d $params{directory} );

    unless (
        Sympa::Tools::File::set_file_rights(
            file  => $params{directory},
            user  => $params{user},
            group => $params{group},
        )
        ) {
        Sympa::Log::Syslog::do_log( 'err', 'Unable to set rights on %s',
            $params{directory} );
        return undef;
    }

    my $pid_file = _get_pid_file(%params);

    my @pids;

    # Lock pid file
    my $lock = Sympa::Lock->new(
        path   => $pid_file,
        method => $params{method},
        user   => $params{user},
        group  => $params{group},
    );
    unless ( defined $lock ) {
        Sympa::Log::Syslog::do_log( 'err', 'Lock could not be created.' );
        return;
    }
    $lock->set_timeout(5);
    unless ( $lock->lock('write') ) {
        Sympa::Log::Syslog::do_log( 'err',
            'Unable to lock %s file in write mode.', $pid_file );
        return;
    }
    ## If pidfile exists, read the PIDs
    if ( -f $pid_file ) {

        # Read pid file
        open( PFILE, '<', $pid_file );
        my $l = <PFILE>;
        close PFILE;
        @pids = grep {/[0-9]+/} split( /\s+/, $l );
    }

    ## If we can have multiple instances for the process.
    ## Print other pids + this one
    if ( $params{multiple_process} ) {
        unless ( open( PIDFILE, '>', $pid_file ) ) {
            ## Unlock pid file
            $lock->unlock();
            Sympa::Log::Syslog::do_log( 'err', 'Could not open %s: %s',
                $pid_file, $ERRNO );
            return;
        }
        ## Print other pids + this one
        push( @pids, $params{pid} );
        print PIDFILE join( ' ', @pids ) . "\n";
        close(PIDFILE);
    } else {
        ## Create and write the pidfile
        unless ( open( PIDFILE, '+>>', $pid_file ) ) {
            ## Unlock pid file
            $lock->unlock();
            Sympa::Log::Syslog::do_log( 'err', 'Could not open %s: %s',
                $pid_file );
            return;
        }
        ## The previous process died suddenly, without pidfile cleanup
        ## Send a notice to listmaster with STDERR of the previous process
        if ( $#pids >= 0 ) {
            my $other_pid = $pids[0];
            Sympa::Log::Syslog::do_log( 'notice',
                "Previous process %s died suddenly ; notifying listmaster",
                $other_pid );
            send_crash_report(
                directory => $params{directory},
                daemon    => $params{daemon},
                pid       => $other_pid,
            );
        }

        #unless ( open( PIDFILE, '>', $pid_file ) ) {
        #    ## Unlock pid file
        #    $lock->unlock();
        #    Sympa::Log::Syslog::do_log( 'err', 'Could not open %s',
        #        $pid_file );
        #    return;
        #}
        unless ( seek( PIDFILE, 0, 0 ) and truncate( PIDFILE, 0 ) ) {
            ## Unlock pid file
            $lock->unlock();
            Sympa::Log::Syslog::do_log( 'err', 'Could not truncate %s.',
                $pid_file );
            return;
        }

        print PIDFILE $params{pid} . "\n";
        close(PIDFILE);
    }

    unless (
        Sympa::Tools::File::set_file_rights(
            file  => $pid_file,
            user  => $params{user},
            group => $params{group}
        )
        ) {
        ## Unlock pid file
        $lock->unlock();
        Sympa::Log::Syslog::do_log( 'err', 'Unable to set rights on %s',
            $pid_file );
        return;
    }
    ## Unlock pid file
    $lock->unlock();

    return 1;
}

#...

# Revised
package Sympa::Tools::File;

use Sympa::Exception
    'Sympa::Exception::File',
    ;

#...

=over

=item set_file_rights(%parameters)

Sets owner and/or access rights on a file.

Throws an exception Sympa::Exception::File
if something went wrong.

=back

=cut

sub set_file_rights {
    my %param = @_;
    my ( $uid, $gid );

    if ( $param{'user'} ) {
        unless ( $uid = ( getpwnam( $param{'user'} ) )[2] ) {
            Sympa::Exception::File->throw(
                sprintf "User %s can't be found in passwd file",
                $param{'user'} );
        }
    } else {

        # A value of -1 is interpreted by most systems to leave that value
        # unchanged.
        $uid = -1;
    }
    if ( $param{'group'} ) {
        unless ( $gid = ( getgrnam( $param{'group'} ) )[2] ) {
            Sympa::Exception::File->throw( sprintf "Group %s can't be found",
                $param{'group'} );
        }
    } else {

        # A value of -1 is interpreted by most systems to leave that value
        # unchanged.
        $gid = -1;
    }
    unless ( chown( $uid, $gid, $param{'file'} ) ) {
        Sympa::Exception::File->throw(
            sprintf "Can't give ownership of file %s to %s.%s: %s",
            $param{'file'}, $param{'user'}, $param{'group'}, "$!" );
    }
    if ( $param{'mode'} ) {
        unless ( chmod( $param{'mode'}, $param{'file'} ) ) {
            Sympa::Exception::File->throw(
                "Can't change rights of file %s to %o: %s",
                $param{'file'}, $param{'mode'}, "$!" );
        }
    }
}

#...

# Revised
package Sympa::Upgrade;

#...

=over

=item to_utf8($files)

Used to encode files to UTF-8. also add X-Attach header field if template
requires it

Parameters:

=over

=item arrayref

A list of filepath/lang pairs

=back

=back

=cut

sub to_utf8 {
    my $files = shift;

    my $with_attachments = qr{ archive.tt2 | digest.tt2 | get_archive.tt2 |
            listmaster_notification.tt2 | message_report.tt2 | moderate.tt2 |
            modindex.tt2 | send_auth.tt2 }x;
    my $total;

    foreach my $pair ( @{$files} ) {
        my ( $file, $lang ) = @$pair;
        unless ( open( TEMPLATE, $file ) ) {
            Sympa::Log::Syslog::do_log( 'err', "Cannot open template %s",
                $file );
            next;
        }

        my $text     = '';
        my $modified = 0;

        ## If filesystem_encoding is set, files are supposed to be encoded 
according to it
        my $charset;
        if ( ( defined $Conf::Conf::Ignored_Conf{'filesystem_encoding'} ) &
            ( $Conf::Conf::Ignored_Conf{'filesystem_encoding'} ne 'utf-8' ) )
        {
            $charset = $Conf::Conf::Ignored_Conf{'filesystem_encoding'};
        } else {
            Sympa::Language::PushLang($lang);
            $charset = Sympa::Language::GetCharset;
            Sympa::Language::PopLang;
        }

        # Add X-Sympa-Attach: headers if required.
        if (   ( $file =~ /mail_tt2/ )
            && ( $file =~ /\/($with_attachments)$/ ) ) {
            while (<TEMPLATE>) {
                $text .= $_;
                if (m/^Content-Type:\s*message\/rfc822/i) {
                    while (<TEMPLATE>) {
                        if (m{^X-Sympa-Attach:}i) {
                            $text .= $_;
                            last;
                        }
                        if (m/^[\r\n]+$/) {
                            $text .= "X-Sympa-Attach: yes\n";
                            $modified = 1;
                            $text .= $_;
                            last;
                        }
                        $text .= $_;
                    }
                }
            }
        } else {
            $text = join( '', <TEMPLATE> );
        }
        close TEMPLATE;

        # Check if template is encoded by UTF-8.
        if ( $text =~ /[^\x20-\x7E]/ ) {
            my $t = $text;
            eval { Encode::decode( 'UTF-8', $t, Encode::FB_CROAK ); };
            if ($@) {
                eval {
                    $t = $text;
                    Encode::from_to( $t, $charset, "UTF-8",
                        Encode::FB_CROAK );
                };
                if ($@) {
                    Sympa::Log::Syslog::do_log( 'err',
                        "Template %s cannot be converted from %s to UTF-8",
                        $charset, $file );
                } else {
                    $text     = $t;
                    $modified = 1;
                }
            }
        }

        next unless $modified;

        my $date = strftime( "%Y.%m.%d-%H.%M.%S", localtime(time) );
        unless ( rename $file, $file . '@' . $date ) {
            Sympa::Log::Syslog::do_log( 'err',
                "Cannot rename old template %s", $file );
            next;
        }
        unless ( open( TEMPLATE, ">$file" ) ) {
            Sympa::Log::Syslog::do_log( 'err', "Cannot open new template %s",
                $file );
            next;
        }
        print TEMPLATE $text;
        close TEMPLATE;
        eval {
            Sympa::Tools::set_file_rights(
                file  => $file,
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
                mode  => 0644,
            );
        };
        if ( my $e = Sympa::Exception->catch ) {
            if ( $e->isa 'Sympa::Exception::File' ) {
                Sympa::Log::Syslog::do_log( 'err', '%s', $e->full_message );
            } else {
                $e->throw;
            }
            next;
        }
        Sympa::Log::Syslog::do_log( 'notice',
            'Modified file %s ; original file kept as %s',
            $file, $file . '@' . $date );

        $total++;
    }

    return $total;
}

#...

# Revised
package Sympa::Configuration;

#...

=over

=item checkfiles_as_root()

Check required files and create them if required

Throws exception if something went wrong.

=back

=cut

sub checkfiles_as_root {

    my $config_err = 0;

    ## Check aliases file
    unless ( -f $Conf{'sendmail_aliases'}
        || ( $Conf{'sendmail_aliases'} =~ /^none$/i ) ) {
        unless ( open ALIASES, ">$Conf{'sendmail_aliases'}" ) {
            Sympa::Log::Syslog::do_log(
                'err',
                "Failed to create aliases file %s",
                $Conf{'sendmail_aliases'}
            );

# printf STDERR "Failed to create aliases file %s", $Conf{'sendmail_aliases'};
            return undef;
        }

        print ALIASES
            "## This aliases file is dedicated to Sympa Mailing List 
Manager\n";
        print ALIASES
            "## You should edit your sendmail.mc or sendmail.cf file to 
declare it\n";
        close ALIASES;
        Sympa::Log::Syslog::do_log(
            'notice',
            "Created missing file %s",
            $Conf{'sendmail_aliases'}
        );
        eval {
            Sympa::Tools::File::set_file_rights(
                file  => $Conf{'sendmail_aliases'},
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
                mode  => 0644,
            );
        };
        if ( my $e = Sympa::Exception->catch ) {

            if ( $e->isa 'Sympa::Exception::File' ) {
                Sympa::Log::Syslog::do_log( 'err', '%s', $e->full_message );
            } else {
                $e->throw;
            }
            return undef;
        }
    }

    foreach my $robot ( keys %{ $Conf{'robots'} } ) {

        # create static content directory
        my $dir = get_robot_conf( $robot, 'static_content_path' );
        if ( $dir ne '' && !-d $dir ) {
            unless ( mkdir( $dir, 0775 ) ) {
                Sympa::Log::Syslog::do_log( 'err',
                    'Unable to create directory %s: %s',
                    $dir, $ERRNO );
                printf STDERR 'Unable to create directory %s: %s', $dir,
                    $ERRNO;
                $config_err++;
            }

            eval {
                Sympa::Tools::File::set_file_rights(
                    file  => $dir,
                    user  => Sympa::Constants::USER,
                    group => Sympa::Constants::GROUP,
                );
            };
            if ( my $e = Sympa::Exception->catch ) {
                if ( $e->isa 'Sympa::Exception::File' ) {
                    Sympa::Log::Syslog::do_log( 'err', '%s',
                        $e->full_message );
                } else {
                    $e->throw;
                }
                return undef;
            }
        }
    }

    return 1;
}

#...

# Revised
package Sympa::Tools::Daemon;

#...

# These throw exceptions on failure.
use Sympa::Exception::AutoDie qw(close open seek truncate);

#...

=over

=item write_pid(%parameters)

Parameters:

=over

=item C<directory> => path

The PID file directory.

=item C<daemon> => string

The daemon name.

=item C<pid> => string

The daemon PID.

=item C<multiple_process> => boolean

FIXME

=item C<method> => string

The PID file locking method.

=item C<user> => string

The PID file user.

=item C<group> => string

The PID file group.

=back

Throws exceptions if something went wrong.

=back

=cut

sub write_pid {
    my (%params) = @_;

    ## Create piddir
    mkdir( $params{directory}, 0755 ) unless ( -d $params{directory} );

    eval {
        Sympa::Tools::File::set_file_rights(
            file  => $params{directory},
            user  => $params{user},
            group => $params{group},
        );
    };
    if ( my $e = Sympa::Exception->catch ) {
        rmdir $params{directory};
        $e->throw;
    }

    my $pid_file = _get_pid_file(%params);

    my @pids;

    # Lock pid file
    # NOTE: new(), set_timeout() and lock() are assumed to throw exception on
    # failure.
    my $lock = Sympa::Lock->new(
        path   => $pid_file,
        method => $params{method},
        user   => $params{user},
        group  => $params{group},
    );
    $lock->set_timeout(5);
    $lock->lock('write');

    ## If pidfile exists, read the PIDs
    if ( -f $pid_file ) {

        # Read pid file
        open( PFILE, '<', $pid_file );
        my $l = <PFILE>;
        close PFILE;
        @pids = grep {/[0-9]+/} split( /\s+/, $l );
    }

    eval {
        ## If we can have multiple instances for the process.
        ## Print other pids + this one
        if ( $params{multiple_process} ) {
            open( PIDFILE, '>', $pid_file );

            ## Print other pids + this one
            push( @pids, $params{pid} );
            print PIDFILE join( ' ', @pids ) . "\n";
            close(PIDFILE);
        } else {
            ## Create and write the pidfile
            open( PIDFILE, '+>>', $pid_file );

            eval {
                ## The previous process died suddenly, without pidfile cleanup
                ## Send a notice to listmaster with STDERR of the previous 
process
                if ( $#pids >= 0 ) {
                    my $other_pid = $pids[0];
                    Sympa::Log::Syslog::do_log(
                        'notice',
                        "Previous process %s died suddenly ; notifying 
listmaster",
                        $other_pid
                    );
                    send_crash_report(
                        directory => $params{directory},
                        daemon    => $params{daemon},
                        pid       => $other_pid,
                    );
                }

                #unless ( open( PIDFILE, '>', $pid_file ) ) {
                #    ## Unlock pid file
                #    $lock->unlock();
                #    Sympa::Log::Syslog::do_log( 'err', 'Could not open %s',
                #        $pid_file );
                #    return;
                #}
                seek( PIDFILE, 0, 0 );
                truncate( PIDFILE, 0 );

                print PIDFILE $params{pid} . "\n";
            };
            if ( my $e = Sympa::Exception->catch ) {

                ## Close pid file
                close PIDFILE;
                $e->throw;
            }
            ## Close pid file
            close(PIDFILE);
        }

        Sympa::Tools::File::set_file_rights(
            file  => $pid_file,
            user  => $params{user},
            group => $params{group}
        );
    };
    if ( my $e = Sympa::Exception->catch ) {

        ## Unlock pid file
        $lock->unlock();
        $e->throw;
    }
    ## Unlock pid file
    $lock->unlock();

    return 1;
}

#...

# Revised #2
package Sympa::Configuration;

#...

# This throws exception on failure.
use Sympa::Exception::AutoDie qw(close mkdir open);

#...

=over

=item checkfiles_as_root()

Check required files and create them if required

Throws exception if something went wrong.

=back

=cut

sub checkfiles_as_root {

    my $config_err = 0;

    ## Check aliases file
    unless ( -f $Conf{'sendmail_aliases'}
        || ( $Conf{'sendmail_aliases'} =~ /^none$/i ) ) {
        open ALIASES, ">$Conf{'sendmail_aliases'}";

        print ALIASES
            "## This aliases file is dedicated to Sympa Mailing List 
Manager\n";
        print ALIASES
            "## You should edit your sendmail.mc or sendmail.cf file to 
declare it\n";
        close ALIASES;
        Sympa::Log::Syslog::do_log(
            'notice',
            "Created missing file %s",
            $Conf{'sendmail_aliases'}
        );
        Sympa::Tools::File::set_file_rights(
            file  => $Conf{'sendmail_aliases'},
            user  => Sympa::Constants::USER,
            group => Sympa::Constants::GROUP,
            mode  => 0644,
        );
    }

    foreach my $robot ( keys %{ $Conf{'robots'} } ) {

        # create static content directory
        my $dir = get_robot_conf( $robot, 'static_content_path' );
        if ( $dir ne '' && !-d $dir ) {
            eval { mkdir( $dir, 0775 ); };
            if ( my $e = Sympa::Exception->catch ) {
                Sympa::Log::Syslog::do_log( 'err', '%s', $e->full_message );
                print STDERR $e->full_message;
                $config_err++;
            }

            Sympa::Tools::File::set_file_rights(
                file  => $dir,
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
            );
        }
    }
}

#...