Re: [RFC PATCH v4 1/2] selinux: use separate table for initial SID lookup

[Paul Moore <paul@paul-moore.com>]

On Thu, Dec 6, 2018 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Dec 5, 2018 at 9:59 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Fri, Nov 30, 2018 at 10:24 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > This moves handling of initial SIDs into a separate table. Note that the
> > > SIDs stored in the main table are now shifted by SECINITSID_NUM and
> > > converted to/from the actual SIDs transparently by helper functions.
> > >
> > > This change doesn't make much sense on its own, but it simplifies
> > > further sidtab overhaul in a succeeding patch.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  security/selinux/ss/policydb.c |  10 +-
> > >  security/selinux/ss/services.c |  88 +++++++++--------
> > >  security/selinux/ss/services.h |   2 +-
> > >  security/selinux/ss/sidtab.c   | 168 ++++++++++++++++++++-------------
> > >  security/selinux/ss/sidtab.h   |  15 ++-
> > >  5 files changed, 173 insertions(+), 110 deletions(-)
> >
> > Based on a quick look, this looks fine to me, and you've gone a couple
> > rounds with Stephen giving it the okay so I'm merging this into
> > selinux/next, but I've had to do some minor tweaks to make
> > checkpatch.pl happy (line length) and cleanup the whitespace (no
> > double vertical whitespace).
> >
> > Please start running checkpatch.pl on your patches.  I don't care if
> > you run it via a git hook, by hand, or some other method - just do it.
> > It isn't hard and it catches lots of silly mistakes.  I believe this
> > is the second time I've mentioned this to you during this development
> > cycle, if this continues I'm going to start rejecting your patches if
> > they fail checkpatch.pl.
> >
> > I'll be the first to admit that checkpatch.pl isn't perfect, e.g.
> > sometimes fixing lines to 80-chars can make things worse, so if you
> > have any questions about checkpatch.pl errors please ask (preferably
> > on-list so everyone can benefit).
>
> I did run checkpatch.pl and I fixed all ERRORs reported. I also fixed
> most WARNINGs, leaving only line length warnings that I wasn't sure
> were worth fixing.

If you aren't sure, ask.  If you can't ask, or don't want to ask, you
should assume that you need to fix the warnings.  I will say that line
length issues are one of the things I do genuinely care about, so
please do fix those.

As an aside, length length is also a great example of exceptions.
There are some cases where forcing the line length under 80 characters
is not advisable: function calls where splitting the line would leave
the first parameter on a second line, splitting a printf() format
string in an awkward spot, moving one or two closing parens to a
separate line, etc.

> Documentation/process/submitting-patches.rst [1]
> does leave a room for exceptions and I know that at last some
> maintainers strongly prefer slightly longer lines before awkward line
> breaks (and I can see that you left a couple 80+ lines in the final
> commit of the second patch as well). I took a bet on leaving most of
> the lines as-is and now I see it was a bad choice... I'll tweak the
> imaginary knob further in the strict direction, hopefully I'll have a
> better accuracy next time.

As I said, please don't hesitate to ask if you think leaving a
checkpatch violation as-is is preferable.

> As for the double empty line, I agree it shouldn't be there, but I
> didn't spot it because chachpatch.pl isn't complaining about that at
> all on my side. Did you spot it manually or am I missing some
> checkpatch.pl flag?

I spotted it manually during review.  If anyone knows how to get
checkpatch to flag this, let me know.

-- 
paul moore
www.paul-moore.com