Re: [PATCH 0/2] secilc/docs: add syntax highlighting for cil examples

[Nicolas Iooss <nicolas.iooss@m4x.org>]

On Fri, Feb 5, 2021 at 12:20 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Thu, Feb 4, 2021 at 4:28 PM bauen1 <j2468h@googlemail.com> wrote:
> >
> > On 2/4/21 9:17 PM, James Carter wrote:
> > > I notice on page 10:
> > > (block unconfined
> > >     (user user)
> > >    ...
> > >
> > > That the second user (which is the name of the user) is highlighted as
> > > well as the user keyword.
> > >
> > > Similar thing happens further done on page 10 with the rule:
> > > (portcon udp 12345 (unconfined.user object_r unconfined.object ((s0) (s0(c0)))))
> > >
> > > The "user" part of "unconfined.user" is highlighted.
> > >
> > > "unconfined.user" is used in other examples as well.
> > >
> > > Changing the first statement to be (user user1) would be fine, but I
> > > would like "unconfined.user" to remain as is.
> > > I am not sure how hard it would be to fix that.
> > >
> >
> > I thought this would be harder, but just highlighting the first cil keyword in a block is actually very easy, and I can rework the patch to do just that.
> >
> > It becomes more difficult when trying to add (some) highlighting to everything else since keywords are reused as names very often (I do that by design even):
> >
> > Some examples from the guide:
> >
> > (macro all ((type x))
> >     (allow x bin_t (policy.file (execute)))
> >     (allowx x bin_t (ioctl policy.file (range 0x1000 0x11FF)))
> > )
> > (call all (bin_t))
> >
> > (mlsvalidatetrans file (domby l1 h2))
> >
> > (defaultrole char target)
> >
> > (type t1)
> > (allow bb.t2 bb.t1 (policy.file (read write execute))))))
> >
> > In the above e.g. all is name, but is usually a keyword with a very important meaning, so imho it should be highlighted, in a lesser way this also goes for t1, or domby, ...
> >
> > The only way I avoid highlighting _all_ names as keywords is to implement a lot of the CIL grammar in the syntax highlighter, but I'm not really sure if it's worth the effort.
> > It could be done for some keywords, e.g. constraints, filecon.
> >
> > A better alternative might be to just highlight less, e.g. drop `low`, `low-high` keywords entirely.
> >
> > I've uploaded another version with some small fixes and a debug color theme to better show what-is-what:
> >
> > https://glados.bauen1.xyz/misc_stuff_might_disappear/CIL_Reference_Guide2.html
> >
> > and with more keywords removed:
> >
> > https://glados.bauen1.xyz/misc_stuff_might_disappear/CIL_Reference_Guide3.html
> >
> > ; only in version 2: file is considered a keyword (builtin), but this should normally only be the case in filecon statements, here it references a class
> > (mlsvalidatetrans file (domby l1 h2))
> >
> > ; all, t1, low is considered a keyword (builtin), but is a name
> > (call all (bin_t))
> > (type t1)
> > (userlevel u5 low)
> >
> > ; but here t2, t1 are no longer highlighted erroneously.
> > (allow bb.t2 bb.t1 (policy.file (read write execute)))
> >
> > I think Version 3 only has mismatches in the example policy due to usage of `all`, `t1` and `t2` as names, so that might be the way to go.
> >
>
> I like version 3 the best, but I really don't like the color used for
> "self", "object_r", "h1" , etc. It just stands out too much.
>
> Thanks,
> Jim

Thanks for working on this. On my side, I have a minor comment,
related to the second patch:

+    <list name="keywords">
+        <item>auditallow</item>
+        <item>tunableif</item>
+        <item>allow</item>
+        <item>dontaudit</item>
...
I find it easier to maintain if the items were sorted in alphabetical
order (this enables inserting new items if the need comes one day,
without wondering whether the new items should be at the end of the
list or if the order should match the one used in some source
files...). Or, if you want to keep this order, please add comments
describing how the lists were created, in order to ease future
modifications.

Thanks,
Nicolas