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

From: Ondrej Mosnacek <omosnace@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: selinux@vger.kernel.org, Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [RFC PATCH v4 1/2] selinux: use separate table for initial SID lookup
Date: Thu, 6 Dec 2018 10:33:23 +0100	[thread overview]
Message-ID: <CAFqZXNv-k0ukrobnXu4dPEc_wHPZAgdUOPE1826+e4m4aArQYg@mail.gmail.com> (raw)
In-Reply-To: <CAHC9VhRb0J4omofmE8j3sDi+EM8FSXTG2qhyzkuPh8i+SWbFHA@mail.gmail.com>

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. 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 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?

Thank you for still having patience with me and thanks to Stephen for
the reviews! I hope the patches will cause more good than bad from now
on :)

[1] https://www.kernel.org/doc/html/v4.19/process/submitting-patches.html#style-check-your-changes

> [...]

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.