lenses/systemd.aug: Allow "+"(fullprivileges) command flag

[tupyy]

This commit allows the "+"(fullprivileges) command flag along with "-" and "@" flags.

Fixes: #839

[rwmjones]

For S-o-b you should use your full name and email, like Signed-off-by: Cosmin Tupangiu <cosmin@redhat.com> as this is a pseudo-legal declaration.

The reporter wasn't me, it was Yongkui Guo <yoguo@redhat.com>

When applying the patch I see trailing whitespace, please fix as well:

Applying: Allow fullprivileges command flag
.git/rebase-apply/patch:29: trailing whitespace.
     let fullprivileges = [ label "fullprivileges" . Util.del_str "+" ] 
warning: 1 line adds whitespace errors.

I tested it and the tests passed for me and it otherwise looks good.

[tupyy]

@rwmjones done

[rwmjones]

LGTM now

[rwmjones]

@tupyy Could you please rebase this on top of the current head.

[tupyy]

@tupyy Could you please rebase this on top of the current head.

@rwmjones done

[georgehansper]

This issue is a bit bigger than it seems

The problem described in issue #839 is correct, and the fix in Pull Request #841 fixes the issue described

However, there a 3 issues which are not addressed, and probably should be

• flag order
  systemd does not consider the order of the flags significant, but this lens does
  This is a characteristic of the existing lens. ie. This problem not introduced bt this change, but it is perpetuated by it
  eg while this lens will read and generate a line

    ExecStart=+-/usr/sbin/sshd -D $OPTIONS

this lens will neither read nor generate a line with the flags in the reverse order

    ExecStart=-+/usr/sbin/sshd -D $OPTIONS`

• unhandled flags
  There are other flags that could be considered - '!' '!!' and ':'
  Note that the systemd man page states that only one of '+' '!' and '!!' may be used.
  While it might be nice to have this restriction taken into account, it is not up to augeas to enforce the syntax rules of the target software, and it would probably contribute significantly to the complexity of the lens

• backwards compatability
  Using the existing lens, some people may be using statements such as

    set /files/lib/systemd/system/sshd.service/Service/ExecStart/command "+/usr/sbin/sshd"

I'm not sure if it's possible, but it would be nice if the updated lens would still accept the old set statement
I do notice that the existing lens does not accept a value of "-/usr/sbin/sshd" so this may not be possible to achieve

[tupyy]

Hello,
As already stated, enforcing systemd cmd flags rules in Augeus, in my opinion, is not very helpful. It should be systemd that check and enforce these flags.
That being said, I don't think it's possible to write lens for the flags that check and enforce the rules. Currently, every flag it's a lens that complicates the writing of +- or -+. At least, I don't know how to write them. I've tried but the problem is Augues complains that the multiple lenses matched the flag.

The easiest way is to bundle everything together like let cmd_flags = /[+-!@:]+/ and let systemd handle the rules.
But this, I think it will break the backwards compatibility.

[georgehansper]

Hi Cosmin,

I think we are agreed that Augeas should not try to enforce systemd flag rules. I only mentioned it to say not to waste effort on this.

I like the idea of having cmd_flags and an explicit value (as you have shown), but as you mention, we should try to keep the lens backwards compatible.

Allowing an arbitrary flag order can be achieved by changing the following 2 lines:

-let value_cmd_re = /[^#@+ \t\n\\-][^#@+ \t\n\\-][^# \t\n\\]*/
+let value_cmd_re = /[^-#@+ \t\n\\][^# \t\n\\]*/

and

-  in fullprivileges? . exit? . arg0?
+  in (fullprivileges | exit |  arg0 )*

Notice that by restricting just the 1st character of value_cmd_re to being a non-flag character, this creates an boundary between the flags and the command. Augeas can use this to unambiguously distinguish the start of the command

[tupyy]

Hi George,

Thank you for the suggestions. I've updated the PR accordingly.
PTAL

Cosmin