Code review comment for lp:~mdeslaur/upstart/apparmor-support

Revision history for this message
James Hunt (jamesodhunt) wrote :

This is excellent. I really like the way you've introduced the new security process type as it is extensible and allows any apparmor_parser failure output to be logged automatically ;-)

My only comment is that we could do with on more test to assert that we can handle reading an "entire" state file that does _not_ contain the new AppArmor-related serialisation data. Specifically, can you add an explicit test to init/tests/test_state.c:test_data_files that:

1) Reads in a prepared JSON file without the AppArmor content.
2) Asserts 'assert0 (state_from_string (json_string));'
3) Checks that JobClass->apparmor_switch has a reasonable value.
4) Checks that Job->pid[PROCESS_SECURITY] has a reasonable value.

Also, a reminder-to-self:

Once lp:~jamesodhunt/upstart/serialise-remaining-objects lands, we'll need to bump STATE_VERSION to take account of the new serialisation format (PROCESS_SECURITY and JobClass->apparmor_switch).

« Back to merge proposal