[apparmor] [patch] split off serialize_parse_profile_start_line()

[Steve Beattie]

On Tue, Mar 10, 2015 at 07:56:43PM +0100, Christian Boltz wrote:
> Am Montag, 9. März 2015 schrieb Steve Beattie:
> > On Tue, Mar 03, 2015 at 11:43:28PM +0100, Christian Boltz wrote:
> > > this patch splits off serialize_parse_profile_start_line() from 
> > > serialize_profile_from_old_profile() in aa.py, as a preparation to
> > > add  tests and then switch to the upcoming RE_PROFILE_START wrapper
> > > function.
> 
> > I'm okay with this as is, so Acked-by: Steve Beattie <steve at nxnw.org>
> > for trunk and 2.9.
> > 
> > However, a couple of things make me scratch my head a bit:
> > 
> >   1) I can't tell entirely from the complicated regex and from the
> > code paths, but is it possible that the boolean value of
> > 'in_contained_hat' can be derived from the other return values?
> 
> Good question, I'd have to dig deeper in the code to answer that.
> 
> >   2) I don't get the purpose of the correct variable and wonder if
> >      we should be raising an exception here?
> 
> Maybe the variable "just" has a strange name, but again, I'd have to dig 
> much deeper to answer that in a correct ;-) way. A quick comparison with 
> parse_profile_start() indicates that "correct == False" could mean an 
> external hat, but without the prof_data_external flag set.
> 
> The tests added in the next patch probably explain the behaviour best - 
> "correct == False" happens
> - for external hats ("/foo//bar {") - which actually _is_ correct syntax
> - for external hats inside a profile (this one is probably worth an 
>   exception, see the comment in the tests)
> 
> This means we should probably rename "correct" to "external_hat" and add 
> an exception if we find an external hat definition inside a profile 
> (that will probably never be hit because invalid syntax like that should 
> already be catched when reading the profiles ;-)
> 
> 
> That said - the purpose of this patch was to split out a part from 
> serialize_profile_from_old_profile() so that I could write tests in a 
> sane way, and to keep the risk if breaking 
> serialize_profile_from_old_profile() (which is not covered by tests yet) 
> as low as possible.
> 
> If we have full test coverage for serialize_profile_from_old_profile() 
> one day, I'll happily hunt down all those gremlins - but without tests, 
> I'm not too keen to do big changes in such a big function ;-)

The reason I was curious and asked was because of the large number
of arguments to serialize_parse_profile_start_line() and the large
number values returned from that function, which is a bit of a code
smell due to code coupling (but then given that it's just pulling
out a chunk of code into a new function, that's not surprising). I
was wondering if we could reduce the number of arguments and return
values by deriving the value of some of them from the other returned
values rather than passing back so many.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150310/fb628b83/attachment.pgp>