[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Wed, 2008-08-27 at 12:31 +0100, Daniel P. Berrange wrote:
> On Tue, Aug 26, 2008 at 09:40:41PM +0000, David Lutterkort wrote:
> > On Tue, 2008-08-26 at 21:05 +0100, Daniel P. Berrange wrote:
> > 
> > > +   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 '[ .. ]'.
> 
> Ok, this gets fun. I don't particularly want the label or tree node.
> I can remove the label OK, but that gives anonymous tree nods. 

Thinking about this some more, anonymous (or labelled) nodes are
probably your best bet here, because:

> If I remove the [...], then I end up with
> 
> test -x /usr/bin/augparse && /usr/bin/augparse -I . test_libvirtd.aug
> ./libvirtd.aug:63.3-.43:Failed to compile lns
> ./libvirtd.aug:63.13-.43:exception: ambiguous tree iteration
>       'ca_file/' can be split into
>       '|=|ca_file/'
> 
>      and
>       'ca_file/|=|'
> 
>     Iterated lens: ./libvirtd.aug:63.15-.39
> 
> test_libvirtd.aug:253.8-.20:Could not load module Libvirtd for Libvirtd.lns
> test_libvirtd.aug:253.8-.20:Undefined variable Libvirtd.lns
> test_libvirtd.aug: error: Loading failed
> 
> And I'm utterly stuck on what this means / how to fix it. I didn't
> expect removing the square brackets to change the DFA.

The error message is admittedly obtuse (though I am not sure how to
improve it, suggestions welcome) 

What Augeas is telling you (or trying to, atleast) is that when it goes
to write a tree back into the file (that's why it mentions 'tree
iteration') it has a choice, and doesn't know what to do in the lens on
line 63, columns 3 - 43, which is the toplevel 'lns' lens:

         let lns = ( record | comment | empty ) *
        
The '|=|' mark in the error message shows where Augeas thinks it could
split a tree with a single node 'ca_file': either into (no node,
cafile/) or (cafile/, no node) .. what makes this error message doubly
heinous is that there's no good notation for 'no node' - it's the empty
string in the error message.

The issue is that when it sees a tree with a single node 'ca_file' it
doesn't know whether to process that as a single 'record' or as a
'record . empty' or 'empty . record', since all of these match the tree
with a single 'ca_file' entry, simply because the 'empty' lens now
leaves no traces in the tree that it had been used. 

When you define 'empty' as '[ eol ]', Augeas knows how to process that
tree since every time 'empty' should be used, there must be a node in
the tree with a NULL label, so a single tree node 'ca_file' can only be
processed by using 'record', and not 'record . empty' or similar.

As I said, the simplest way around this is to define 'empty' as
'[ eol ]' - you could also try to change things so that empty lines are
pulled into record or comment entries, but that's probably more trouble
than it's worth.

> > 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 :(
> 
> Yes, that would be very nice - and/or supporting use of <<HERE documents
> for strings, so I don't have to escape "  throughout the embedded config
> file

One more note on this: to get started, pulling in a whole config file
and checking that that gets processed in a reasonable way is deifnitely
the best thing to do. Over time, as you make changes to the file format
and/or fix bugs in the lens, you're probably better off writing small
tests that check for one particular thing (the test_shellvars.aug in
Augeas hg is a good example for this)

I put a ticket into Trac for this[1]

> One other useful improvement would be for the test suite to only print
> out sections of the tree which differ. In debugging this it printed
> out a 400 line 'Expected' tree structure, and then a 400 line 'Actual'
> tree structure and wanted me to play "Where's Waldo" to find the typo.
> 
> It would be good to trim the leading and trailing nodes which are the
> same

Yeah, I've run into this before, too. Yet another ticket[2]

> > All in all, very nice, and I am really glad that upstream is shipping a
> > lens :)
> 
> Just discovered one minor issue. The Fedora RPM guidelines require that
> an RPM either own a directory, or require an RPM that owns it, and two
> RPMs aren't allowed to own the same directory. I don't want/need to
> depend on Augeas directly, but equally I need someone to own the
> /usr/share/augeas/lens directory. Wonder if that directory should be put
> into the 'filesystem'  RPM  ?

Yeah, that's the only feasible solution. I'll need to figure out what
the process for that is.

David

[1] http://fedorahosted.org/augeas/ticket/10
[2] http://fedorahosted.org/augeas/ticket/11



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]