[augeas-devel] [PATCH] Add sudoers lens and associated test

[Raphaël Pinson]

On Tue, Aug 12, 2008 at 8:49 PM, David Lutterkort <dlutter at redhat.com>wrote:

> On Tue, 2008-08-12 at 11:28 +0200, Raphael Pinson wrote:
> > # HG changeset patch
> > # User Raphael Pinson <raphink at gmail.com>
> > # Date 1218533244 -7200
> > # Node ID fe8236ec9a82e500be65689abd79de394c253e8a
> > # Parent  5697b74b2d7341f923ee889187a9d8d6276744ae
> > Add sudoers lens and associated test
>
> Very nice ... I had no idea Augeas could do all that ;) Having the
> comments for the Sudo grammar in the file makes this very readable.
>

Yes. I'm not used to using boxes in comments really, but in this case it
makes things clearer since there's so many comments. I think putting the
specs from manpages helps understanding the code in the lens.


I have committed this after some reformatting - I really want to keep
> lines to at most 80 characters (I usually stop at 78) I also put all the
> 'let's flush-left to get a few extra characters.
>

I agree with the 80 characters. It is a policy in Debian to cut all lines at
80 characters when packaging and I usually try to do it. I just didn't have
the time to go through it today for this lens, so thanks for doing it. I
think it could be part of the style guidelines we mentioned today.


One question about this:
>
> > +
>  (***********************************************************************************
> > +    *  Parameter ::= Parameter '=' Value |
> > +    *                Parameter '+=' Value |
> > +    *                Parameter '-=' Value |
> > +    *                '!'* Parameter
> > +
>  ***********************************************************************************)
> > +    let parameter        = [ label "parameter" . sto_to_com ]
>
> What do you think about splitting the values from the parameter names in
> the tree ?
>

I thought about that, too. I stayed with a very simple description of it so
far, because I'm not sure how to represent the different attributions.
Something like:

Defaults param1="value1", param2+="value2", param3-="value3", !param4

could become

{ "Defaults"
    { "parameter"
        { "name" = "param1" }
        { "type"   = "=" }
        { "value"  = "value1" } }
    { "parameter"
        { "name" = "param2" }
        { "type"   = "+=" }
        { "value"  = "value2" } }
    { "parameter"
        { "name" = "param3" }
        { "type"   = "-=" }
        { "value"  = "value3" } }
    { "parameter"
        { "name" = "param4" }
        { "negate" = "!" } } }


Somehow I'm not sure it's any easier to use than the way it currently is. It
might even be confusing because there's different fields for key/value pairs
and for flags. Not sure, really...


Also, the above production is not what is actually processed there; it's
> in the sudoers man page like that, but generates the empty language ;)
>
> What they probably mean is
>
> Parameter ::= ParamName '=' Value |
>              ParamName '+=' Value |
>              ParamName '-=' Value |
>              '!'* ParamName
>


Yes, there might be a few issues in the manpages (although they're still one
of the best manpages I've ever used). I've mentionned the lack of spec on
Defaults!Cmnd_List in the lens as well.


> though I don't really understand the manpage at that point.
>


Maybe you're talking about the '!'* part here. This is another thing that
would be quite hard for us to parse, if even possible. The parameter can be
preceded by any amount of '!', each overriding the other. an even amount of
them will cancel the effect of it. It's really this kind of syntax that made
me choose to stay away from analyzing what takes place inside the Parameter
fields.


Raphael
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/augeas-devel/attachments/20080812/68d262c6/attachment.htm>