Developers of Sympa

[David Verdin <address@concealed>]

Dear all,

Le 02/10/13 10:09, IKEDA Soji a écrit :

Hi,

On Thu, 19 Sep 2013 17:35:16 +0200
David Verdin <address@concealed> wrote:

        Logs

  * We will get rid of interpolation in do_log calls (replace
    Sympa::Log::do_log('err', "Could not open $file"); by
    Sympa::Log::do_log('err', 'Could not open %s', $file);)
  * We will inspect logs to distinguish, amongst the different "debug"
    levels, which are relevant to end users and which are not. The first
    will be put in log_message calls, the former in log_trace_message
    calls. There is a problem with this simplification: some functions
    are called very often and, if all debug logs are actually printed,
    in an instance with a big activity, the performances will severely
    decrease. It can even stall the server. So this simple dichotomy
    between developpers and users could be problematic.
  * 'warn' log level is useless. Let's have only two levels: "info" and
    "err". That's enough.
  * Sympa::Log::Database is aimed at providing web-readable logs for
    list owners. It logs only main events to make them available through
    Sympa web interface.
  * the web specific logs are useful because they provide informations
    specific to the web. It is important when analyzing logs. All three
    logging functions have their rational. I don't know how to simplify
    this part of the code.

I'm not confident of unusefulness of 'warn' level, while by now I
can also not prove usefulness of it.  So I'll withdraw it.

On 'debug' level, I don't worry about performance.  Even when logging
is disabled inside log_message() (perhaps because debugging level is
low enough), this function shall be called.  When it was enabled,
decrese of performance is the result of configuration done by
administrator.

Database logging looks to hit performance much for me.  It might
be disabled by a configuration parameter.

Actually, database logs only usage is to let list owners query the logs.
I don't think this functionnality is much used. We could simply get rid of database logging and replace it by simple queries inside filesystem logs.
The problem would be to query rotated logs. Most system have daily log files archived in a way or another. There are a lot of different formalisms and It would be hard to find the relevant files.
Still another solution: Replace relational database logs by filesystem database logs, and handle logs rotation inside Sympa, with a well-defined formalism.

Exception handling seems not to be resolved. the fatal_err calls were 
usefull because they specified explicitely that Sympa should not keep on 
running under certain circumstances. This is a clear distinction with 
the return undef we used as exception handling.
I am under the impression that all return undef could be replaced by 
croak calls, providing we intercept exceptions at the right place and we 
use the Carp option allowing to produce stak traces.
But, we need to distinguish the very few circumstances in which sympa 
should stop running, so that the daemons will stop gracefully. 
Inspecting the code, it looks like all fatal_err calls were made in the 
main daemons, so I think it should be easy.

My concern on exception:

- It tends to skip clean-up processes inside "try" and other blocks.
  Not only skipping each logic, it is likly to cause leaks of
  rosources such as memory.  The function corresponding to "finally"
  may probably ease such situation, while it can worsen the next
  problem.

If the "finally" resolves the memory leaks, I violently support this option.

- It is a bit hard to read.  The idiom using eval {} and if () is
  obvious (additionally, by this idiom, programmers must re-throw
  uncaught exceptions by themselves).  Syntax sugers provided by
  several modules are not always acceptable by average Perl
  programmers.

If we set up clear rules about how exceptions are handled in Sympa on the Sympa web site, it should not be too much trouble.

I'm not an opponent of exception, contrary, I prefer to this
concept.  But by now I'm undecided.

There is something still unclear to me: How exceptions are escalated. If I understand correctly, we plan to croak or carp everywhere we used "return undef" and eval{} these exceptions at the very top level (i.e., in each Sympa daemon loops). In the end, will we have only the original message or all the croak messages we will meet between the original error and the final eval{} interception?

        Code formatting

  * Let's indent with spaces. 4 spaces width.
  * We will not use perltidy in a post-commit hook. it could mess up
    carefully hand-crafted code layout.
  * We should however run perltydy once before continuing bug fixing, so
    that, if some of the layout is messed up, it is at least at a time
    when we have to read the code a lot and potentially fix some abusive
    line feeds.

I have no objection.

Great!

  * do everybody agrees with the following settings?

-bar   # Opening brace always on right (* no)
-bbt=1 # Medium block brace tightness
-bt=1  # Medium brace tightness (* 1)
-ce    # Cuddled else (* no)
-cti=0 # No extra indentation for closing brackets
-et=4  # Entab leading 4 whitespace (* none)
-i=4   # Indent level is 4 cols
-ci=4  # Continuation indent is 8 cols
-l=78  # Max line witdh is 78 cols
-nolq  # Don't outdent long quoted strings
-nsbl  # No opening sub brace on new line (* -sbl)
-nsfs  # No space before semicolons
-pt=1  # Medium parenthesis tightness (* 1)
-sbt=1 # Medium square bracket tightness (* 1)
-se    # Errors to STDERR
#-st   # Output to STDOUT
-vt=2  # Maximal vertical tightness
-wba="% + - * / x != == >= <= =~ !~ < > | & >= < = **= += *= &= <<= &&= -= /= |= >>= ||= .= %= ^= x= || && . ? : and or xor"
# Break after all operators (* not contains "||" and tokens appear after it)

Difference from PBP are:

-ce : Cuddled else (PBP : -nce)

-et=4 : Entab leading 4 whitespace (PBP : none)

-nsbl : No opening sub brace on new line (PBP : -sbl)

-wba : Add following operators
  || && . ? : and or xor


In above, "-et=4" seems to be inconsistent with your proposal.

OK, Let's remove it.

Addition to -wba (by me) sometimes generates a bit misterious
results.

OK, let's remove it.

Cheers,

David

Regards,


--- Soji

--
A bug in Sympa? Quick! To the bug tracker!

David Verdin Études et projets applicatifs
Tél : +33 2 23 23 69 71 Fax : +33 2 23 23 71 21   www.renater.fr	RENATER 263 Avenue du Gal Leclerc 35042 Rennes Cedex

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