On Thu, Dec 6, 2018 at 4:33 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > On Wed, Dec 5, 2018 at 9:59 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Fri, Nov 30, 2018 at 10:24 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > 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
