[augeas-devel] Request for Review

[Raphaël Pinson]

Hello,


On Wed, Oct 26, 2011 at 3:40 PM, Brewer, Janzen H. <
Janzen.Brewer at gtri.gatech.edu> wrote:

>  I'm new to augeas and I'm trying my hand at writing file descriptions.
> I'd really appreciate your feedback on this one. It's written for
> /etc/audit/auditd.conf. I also wrote a test.
>
>

That's a very good start, and both lens and test pass augparse successfully,
congrats!


I'll just add a few comments:


-------------------------
> augeas/lenses/auditd.aug:
> -------------------------
> (* Auditd module for Augeas
>  Author: Janzen Brewer <janzen.brewer at gtri.gatech.edu>
>  Based on Free Ekanayaka's dnsmasq module
>
>  Reference: man auditd.conf (8)
>
>  "[auditd.conf] should contain one configuration keyword per line, an equal
>  sign, and then followed by appropriate configuration information."
>
> *)
>
> module Auditd =
>
>    autoload xfm
>
> (************************************************************************
>  *                           USEFUL PRIMITIVES
>  *************************************************************************)
>
>
It could be nice to use NaturalDocs comments, as seen in e.g.
keepalived.aug. We generate documentation for lenses from the comments. See
e.g. http://augeas.net/docs/references/lenses/files/keepalived-aug.html



> let eol        = Util.eol
> let spc        = Util.del_ws_spc
>


Nowadays, I'd prefer to use Sep.space, which is equivalent, but clearer (in
my opinion).


let comment    = Util.comment
> let empty      = Util.empty
>
> let sep_eq     = del /=/ "="
>

This is Sep.equal.


> let sto_to_eol = store /([^ \t\n].*[^ \t\n]|[^ \t\n])/
>
>
You could use "store Rx.space_in" here.



> (************************************************************************
>  *                               ENTRIES
>  *************************************************************************)
>
> (*let entry_re = /[A-Za-z0-9._-]+/*)
> let key_value (kw:string) = [ key kw . spc . sep_eq . spc . sto_to_eol .
> eol ]
>


This can be written using the build module as:

let eq = del /[ \t]*=[ \t]*/ "="
let key_value (kw:string) = Build.key_value_line kw eq sto_to_eol



> let entry = key_value "log_file"
>           | key_value "log_format"
>           | key_value "log_group"
>           | key_value "priority_boost"
>           | key_value "flush"
>           | key_value "freq"
>           | key_value "num_logs"
>           | key_value "max_log_file"
>           | key_value "max_log_file_action"
>           | key_value "space_left"
>           | key_value "space_left_action"
>           | key_value "action_mail_acct"
>           | key_value "admin_space_left"
>           | key_value "admin_space_left_action"
>           | key_value "disk_full_action"
>           | key_value "disk_error_action"
>           | key_value "dispatcher"
>           | key_value "disp_qos"
>           | key_value "name"
>           | key_value "name_format"
>           | key_value "tcp_listen_port"
>           | key_value "tcp_listen_queue"
>           | key_value "use_libwrap"
>           | key_value "tcp_client_ports"
>           | key_value "tcp_client_max_idle"
>           | key_value "tcp_max_per_addr"
>           | key_value "enable_krb5"
>           | key_value "krb5_principal"
>           | key_value "krb5_key_file"
>



But since Build.key_value_line accepts regexps as keys, I would suggest to
define the keys as a union of regexps and pass it to key_value_line in one
go. This will make a more efficient regexps, and simplify your code:


let entry_re =  "log_file"
             | "log_format"
             | "log_group"
             | "priority_boost"
             | "flush"
             | "freq"
             | "num_logs"
             | "max_log_file"
             | "max_log_file_action"
             | "space_left"
             | "space_left_action"
             | "action_mail_acct"
             | "admin_space_left"
             | "admin_space_left_action"
             | "disk_full_action"
             | "disk_error_action"
             | "dispatcher"
             | "disp_qos"
             | "name"
             | "name_format"
             | "tcp_listen_port"
             | "tcp_listen_queue"
             | "use_libwrap"
             | "tcp_client_ports"
             | "tcp_client_max_idle"
             | "tcp_max_per_addr"
             | "enable_krb5"
             | "krb5_principal"
             | "krb5_key_file"

let entry = Build.key_value_line entry_re eq sto_to_eol


The result in terms of performance, using the current code:

time augparse -I . tests/test_auditd.aug

real    0m0.780s
user    0m0.632s
sys    0m0.088s


and using the new code:

time augparse -I . tests/test_auditd.aug

real    0m0.139s
user    0m0.076s
sys    0m0.012s


You can also get rid of sep_eq (or redefine it if you prefer that variable
name, I just wanted to make it clear that I used another variable here),
spc, and eol.

Note also that entry_re could be simply Rx.word. You would allow incorrect
keywords, but it would parse fine and it would be easier to maintain the
lens in the future.



Cheers,


Raphaël
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/augeas-devel/attachments/20111026/7b3c07a5/attachment.htm>