Code review comment for lp:~cwayne/ubuntu-touch-customization-hooks/more-apparmor-copy

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Chris and I talked about this a bit. Policy can be recompiled when:
* the symlink in /var/lib/apparmor/clicks is newer than the profile in /var/lib/apparmor/profiles
* the profile in /var/lib/apparmor/profiles is newer than the cache file in /var/cache/apparmor

The click system hook upstart job adds the symlinks to /var/lib/apparmor/clicks and the click apparmor system hook then creates policy in /var/lib/apparmor/profiles based on this symlink. What was happening with previous attempts of the upstart job is that /var/lib/apparmor/clicks and /var/lib/apparmor/profiles were empty. This meant that even though this job copied the cache files into place correctly and at the right time, the click system hook machinery created newer symlinks and profiles than the cache, so the cache files were ignored. Previous job testing involved removing only the cache files in /var/cache/apparmor and rebooting, which left the already creating symlinks and profiles in place which is why it worked correctly on reboot.

I agree with the approach set out by this merge request. The script should probably also check for the existence of /var/lib/apparmor/clicks and /var/lib/apparmor/profiles. Also, '|| true' is not included in the other cp statements. Maybe something more like this (please include the comment):

# Ordering is important. mtimes for the following must be:
# click symlink < apparmor profile < apparmor cache
if [ -d /custom/lib/apparmor/clicks ] && [ -d /var/lib/apparmor/clicks ] ; then
    cp -Pnu /custom/lib/apparmor/clicks/* /var/lib/apparmor/clicks || true
fi
if [ -d /custom/lib/apparmor/profiles ] && [ -d /var/lib/apparmor/profiles ] ; then
    cp -nu /custom/lib/apparmor/profiles/* /var/lib/apparmor/profiles || true
fi
if [ -d /custom/cache/apparmor ] && [ -d /var/cache/apparmor ] ; then
    cp -nu /custom/cache/apparmor/* /var/cache/apparmor/ || true
fi

review: Needs Fixing

« Back to merge proposal