[augeas-devel] Request for Review

[Janzen Brewer]

Thanks for the tips, Raphaël.

Janzen

On 10/26/2011 09:59 AM, Raphaël Pinson wrote:
> Hello,
>
>
> On Wed, Oct 26, 2011 at 3:40 PM, Brewer, Janzen H.
> <Janzen.Brewer at gtri.gatech.edu <mailto: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
>     <mailto: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
>