On Fri, Jun 9, 2023 at 4:24 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > Hello all, > > Over the past few weeks, as time allowed, I've been working on adding > some automation to my Linux Kernel workflows. Most of this has been > scripting various maintainer tasks, e.g. reviewing and merging > patches, which I expect is not very interesting to most people reading > this, but I think there is one aspect of these automations that might > be interesting for a wider audience: basic patch sanity/verification > tests. These tests are intended to mimic the sanity tests I perform > when reviewing patch submissions, things like "run checkpatch", "make > sure it builds without error", "ensure the style/formatting is > reasonable", etc. For those that are curious, you can see the current > tests in the repo below, but I will caution you that there are surely > problems with the scripts, they are still very new and barely tested; > expect changes. I should also note that I haven't published the > tool/framework which I use to run these tests just yet, but the tests > are intended to be standalone so there should still be value as-is. > My thinking is that by sharing these scripts, and keeping them > updated, it will help keep the developers and reviewers sync'd as to > what is expected from a SELinux kernel patch. Much like checkpatch, I > don't expect these scripts to represent a perfect, ideal standard but > I think they represent a "good enough" example where the accepted > verification failures should be relatively few. > > * https://github.com/pcmoore/git-verification_scripts This looks like a nice step forward. Are you also looking into integrating these checks into patchwork? I noticed that e.g. in the netdev project patches get tagged with similar sanity checks (netdev) and even test results (bpf). Perhaps you could even reuse some of the tooling/infra they use. > With all of the above in mind, I wanted to get everyone's opinions on > the style/formatting suggested by the scripts above. As anyone who > has looked at the SELinux kernel code knows, the style is somewhat > inconsistent, both with respect to the SELinux subsystem, and the > kernel as a whole. That unfortunately means that if we want to be > able to start vetting the style of new code changes, we really need to > properly (re)format the existing code first. Before I went too far > with this I wanted to see what it might look like when applied to one > of our ugliest source files, security/selinux/hooks.c; you can see the > results in the commit below: > > * https://github.com/pcmoore/misc-linux_kernel/commit/3f94fd77b46522a038eb6771b63d0a6d36ca3547 > > I'd like to hear what everyone thinks about making a change like this. > I personally think it is a positive step forward, but I do acknowledge > that those who have to do backports will likely feel some occasional > pain. As the backport pain will eventually subside, and the benefits > of nicer-looking code and improved/shared patch verification will > continue, I'm leaning towards reformatting the code, but I do *really* > want to hear what people have to say before we do this. Regarding the impact on backports: yes, this will be potentially *very* painful for us (Red Hat engineering), depending on how it's done. What would help is if you could split the change into many small commits - ideally something close to per-function (with some closely related functions/declarations/definitions maybe combined together). I know it would be a lot of commits and a non-trivial amount of extra work, but from a consumer's perspective I don't really see any downsides vs. a single big commit. From a pure upstream perspective I definitely support the change, so hopefully we can come to a compromise such that we are both happy with the result :) As for the proposed style, it looks mostly good to me. One minor concern I have is that it removes some visually helpful alignment after '='/':' in selinux_sb_clone_mnt_opts(), selinux_syslog(), selinux_fs_parameters, and selinux_nf_ops. Not something I would lose sleep over, but would be nice if it can be somehow preserved. -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
