On 2015-01-22 10:12:25, John Johansen wrote: > On 12/05/2014 04:21 PM, Tyler Hicks wrote: > > This patch set is the precursor to moving the cache loading code into > > libapparmor. It sets up a clean, supportable interface for external > > programs to interact with match files, feature files, the policy cache, > > and the kernel interface (apparmorfs). It doesn't provide an API for > > individual binary cache files. That will come in the future but > > shouldn't be needed yet as this patch set provides a way to load all > > binaries in the policy cache. > > > > Code that is being moved into libapparmor must not print to > > stdout/stderr and it must not make calls such as exit(). A good portion > > of this patch set is shuffling code around so that error codes can be > > returned up the stack instead of printing to stderr and calling exit(). > > Unfortunately, some useful user-facing logging and developer-facing > > debug logging must be dropped. errno values are used to convey the error > > back up the stack. The use of errno is probably my least favorite thing > > about the patch set. > > > > The new APIs mimic those found in GLib with _new(), _ref(), and _unref() > > functions that operate on opaque objects. This style is used so that we > > can, for example, change what our underlying policy cache looks like > > without breaking our ABI. This is important as we're looking to make > > some considerably large changes to the policy cache in the near term. > > > > There are a few warts in this patch set: > > > > 1) The most obvious is that the cache loading functions aren't yet in > > libapparmor. This patch set lays the groundwork and has the > > potential to be controversial. There's just a little more work left > > to move the functions over and I've got a decent start on it. > > 2) Objects created from the new APIs aren't properly unref'ed/freed. > > This is because there are still quite a few exit()'s being called in > > apparmor_parser. More refactoring needs to be done before we can be > > sure that cleanup is done before the process dies. Luckily, the new > > aa_*_unref() functions only free memory (as compared to unlinking a > > file, for example) and there are many pre-existing memory > > allocations that aren't currently being freed at exit(). > > 3) As mentioned above, individual binary policy files aren't > > represented in the API. That's not too important until we start > > looking at moving the policy compilation code over to libapparmor. > > Right now, saying "load all binaries in the policy cache" should be > > sufficient. > > > > This patch set passes the parser's make check tests (including the > > caching tests). I also built a parser against a Utopic libapparmor, > > copied the new parser into /sbin/, and did a successful QRT > > test-apparmor.py run which includes a wide array of tests. I think it > > should be pretty solid. > > > > Sorry for the patch set growing so large. John initially handed me a set > > of 10 or more patches to begin with and then I tried to follow his lead > > and just kept coming across more and more that needed to be done. > > > Sorry this took so long. I have gone through all the patches including > the first 10 that I dumped on Tyler, those all looked good to me but I > am not going to stick an acked-by on patches I already have a signed-off-by > on.
There is a lot to digest and the time that I sent them out was less than ideal since the holidays were approaching. I agree that acked-by does not make sense for where we already have our signed-off-by. > > Tyler your changes to those patches look fine, and I think our dual signed-off > is enough for those. I thought that dual sign-off was sufficient, too. If anyone has a problem with that, please speak up and start reviewing. ;) > So I think my big issue with the series is really just questions around > the api and where we want it to go. Not all of that needs to be determined > now, but I'd like a few other people to kick in their ideas Yep - the actual code changes are intended to be minimal. The API changes should be the focus of the review, IMO. Tyler
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
