[libvirt] Re: [augeas-devel] PATCH: Add a augeas lens for libvirtd.conf

[David Lutterkort]

On Tue, 2008-08-26 at 21:05 +0100, Daniel P. Berrange wrote:
> Now instead of telling people 
> 
>   'edit /etc/libvirt/libvirtd.conf and change listen_tls to 1,
>    and auth_tls to sasl'
> 
> we can say run
> 
>    # augtool <<EOF
>    set /files/etc/libvirt/libvirtd.conf/listen_tls 1
>    set /files/etc/libvirt/libvirtd.conf/auth_tls sasl
>    save
>    EOF

Very nice.

> THis patch is intended to be committed to libvirt, so the config file rules
> are distributed alongside libvirt. I'm CC'ing augeas-devel for feedback on
> the lens itself.
> 
>  libvirt.spec.in         |    2 
>  qemud/Makefile.am       |    8 
>  qemud/libvirtd.aug      |   64 ++++++
>  qemud/test_libvirtd.aug |  484 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 558 insertions(+)
> 
> 
> Daniel
> 
> Index: qemud/Makefile.am
> ===================================================================
> RCS file: /data/cvs/libvirt/qemud/Makefile.am,v
> retrieving revision 1.51
> diff -u -p -r1.51 Makefile.am
> --- qemud/Makefile.am	20 Aug 2008 20:48:35 -0000	1.51
> +++ qemud/Makefile.am	26 Aug 2008 20:03:48 -0000
> @@ -24,6 +24,8 @@ EXTRA_DIST =						\
>  	libvirtd.policy					\
>  	libvirtd.sasl					\
>  	libvirtd.sysconf				\
> +        libvirtd.aug                                    \
> +        test_libvirtd.aug                               \
>  	$(AVAHI_SOURCES)				\
>  	$(DAEMON_SOURCES)
>  
> @@ -56,6 +58,12 @@ sbin_PROGRAMS = libvirtd
>  confdir = $(sysconfdir)/libvirt/
>  conf_DATA = libvirtd.conf
>  
> +augeasdir = $(datadir)/augeas/lenses
> +augeas_DATA = libvirtd.aug
> +
> +augeastestsdir = $(datadir)/augeas/lenses/tests
> +augeastests_DATA = test_libvirtd.aug
> +
>  libvirtd_SOURCES = $(DAEMON_SOURCES)

You might also add a test that runs 
        augparse -I . test_libvirtd.aug
during 'make check'

>  #-D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L
> Index: qemud/libvirtd.aug
> ===================================================================
> RCS file: qemud/libvirtd.aug
> diff -N qemud/libvirtd.aug
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ qemud/libvirtd.aug	26 Aug 2008 20:03:48 -0000
> @@ -0,0 +1,64 @@
> +(* /etc/libvirt/libvirtd.conf *)
> +
> +module Libvirtd =
> +   autoload xfm
> +
> +   let eol   = del /[ \t]*\n/ "\n"
> +   let value_sep   = del /[ \t]*=[ \t]*/  " = "
> +   let prespace = del /[ \t]*/ ""
> +
> +   let array_sep  = del /,[ \t\n]*/ ", "
> +   let array_start = del /\[[ \t\n]*/ "[ "
> +   let array_end = del /\]/ " ]"

Augeas should throw an error here, but doesn't ;) The default value you
give as the second argument of del really should match the first
(regexp) argument.

Also, Augeas automatically promotes strings to regexps as needed, so you
can say

        let array_end = del "]" "]"

> +   let str_val = del /\"/ "\"" . store /[^\"]*/ . del /\"/ "\""
> +   let bool_val = store /0|1/
> +   let str_array_element = [ str_val ] . del /[ \t\n]*/ ""
> +   let str_array_val = array_start . ( str_array_element . ( array_sep . str_array_element ) * ) ? . array_end

You should really have some sort of label on each array element/str val
(either the same for all of them using 'label' or just consecutive
numbers using 'seq') - without that, you won't be able to get to
individual entries through the public API.

> +   let comment = [ label "comment" . del /#[ \t]*/ "# " .  store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ]

We've been trying to label all comments as '#comment'; it's not an issue
for libvirt, but with some other file formats, using 'comment' leads to
conflicts with actual entries. Would be good to stick to that
convention.

> +   let empty = [ label "empty" . del /[ \t]*\n/ "" ]

Do you really care that empty entries show up in the tree ? If you don't
want to see them, you can remove the 'label' from the above, and
possibly also the '[ .. ]'.

And the missing check for 'del' strikes again - that needs to be
'del /[ \t]*\n/ "\n"' - I really need to fix that.

> Index: qemud/test_libvirtd.aug
> ===================================================================
> RCS file: qemud/test_libvirtd.aug
> diff -N qemud/test_libvirtd.aug
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ qemud/test_libvirtd.aug	26 Aug 2008 20:03:48 -0000
> @@ -0,0 +1,484 @@
> +module Test_libvirtd =
> +   let conf1 = "# Master libvirt daemon configuration file

I've been thinking that we need to have some function that reads a file
and returns a string so we don't have to clutter tests with these long
strings. But for now, that's what it is :(

> +        { "tls_allowed_dn_list"
> +             { = "DN1"}
> +             { = "DN2"}
> +        }

This happens because str_array_element produces tree nodes without
labels. Users won't have a way to change e.g. the 'DN1' value to 'myDN'
since you can't address a node without a label through the public API.

All in all, very nice, and I am really glad that upstream is shipping a
lens :)

David