Developers of Sympa

[Guillaume Rousse <address@concealed>]

Le 21/10/2013 15:18, IKEDA Soji a écrit :
> 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.
As already said, I see no interest of rushing into class-based 
exceptions right now. Especially as the current usage of OO-modeling in 
Sympa codebase is...  largely improvable ? You can achieve exactly the 
same results using a standard croak() or die() function, as long as 
you're simply passing string messages.

For me, the absolute priority is to improve code readability, by 
lowering overall complexity. And to introduce changes progressively, 
once we have a working code base, and a test suite to catch regression.

> - "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.
Agreed.

> 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.
Once again, don't use class-based exceptions, you won't have this issue.

> - 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.
Your error-handling block is over-engineered: you're making the 
assumption than set_file_rights() will now throw any kind of exception, 
whereas your change only introduced the possibility of throwing 
Sympa::Exception::File ones.

Here is an equivalent, but simpler, proposal:

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

    if ($param{'user'}) {
         unless ($uid = (getpwnam($param{'user'}))[2]) {
            die(
                "User %s can't be found in passwd file\n",
                $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]) {
            die("Group %s can't be found\n", $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'})) {
        die(
            "Can't give ownership of file %s to %s.%s: %s\n",
            $param{'file'}, $param{'user'}, $param{'group'}, $!
        );
    }

    if ($param{'mode'}) {
        unless (chmod($param{'mode'}, $param{'file'})) {
            die(
                "Can't change rights of file %s to %o: %s\n",
                $param{'file'}, $param{'mode'}, $!
            );
        }
    }

}

to_utf8 {
...

        eval {
            Sympa::Tools::set_file_rights(
                file  => $file,
                user  => Sympa::Constants::USER,
                group => Sympa::Constants::GROUP,
                mode  => 0644,
            );
        };
        if ($EVAL_ERROR) {
            Sympa::Log::Syslog::do_log( 'err', '%s', $EVAL_ERROR);
            next;
        }

No fancy additional class, and no hypothesis about new exceptions coming 
from nowhere.
--
Guillaume Rousse
INRIA, Direction des systèmes d'information
Domaine de Voluceau
Rocquencourt - BP 105
78153 Le Chesnay
Tel: 01 39 63 58 31

Attachment: smime.p7s
Description: Signature cryptographique S/MIME