[apparmor] [PATCH v4] binutils: Add aa-enabled program to check AppArmor status

Thu Dec 17 00:32:56 UTC 2015

On 2015-12-16 15:21:46, John Johansen wrote:
> On 12/16/2015 03:17 PM, Tyler Hicks wrote:
> > On 2015-12-16 23:46:23, Christian Boltz wrote:
> >> Hello,
> >>
> >> Am Mittwoch, 16. Dezember 2015 schrieb Tyler Hicks:
> >>> diff --git a/README b/README
> >>> index 4ebd25d..4797b78 100644
> >>> --- a/README
> >>> +++ b/README
> >>
> >>> @@ -104,7 +112,7 @@ $ make check	# depends on the parser having been
> >>> built first $ make install
> >>>
> >>>
> >>> -[Note that for the parser and the utils, if you only with to
> >>> build/use 
> >>> +[Note that for the parser, binutils, and utils, if you
> >>> only with to 
> >>
> >> unrelated, but please s/with/wish/
> > 
> > I'll make the change in my local copy of this commit.
> > 
> >>
> >>> build/use some of the locale languages, you can override
> >>> the default by passing the LANGS arguments to make; e.g. make all
> >>> install "LANGS=en_US fr".]
> >>>
> >>> diff --git a/binutils/Makefile b/binutils/Makefile
> >>> new file mode 100644
> >>> index 0000000..c4c4c2e
> >>> --- /dev/null
> >>> +++ b/binutils/Makefile
> >>
> >> My general note that this looks like a copy of parser/Makefile and 
> >> splitting out common parts to something like common/Make-c.rules would 
> >> make sense still stands - but this can be done as a separate patch.
> > 
> > I agree that cleaning up the common Makefile snippets is a good idea.
> > However, it is not work that I signed up for when offering to take over
> > this patch and it isn't something that I'll be able to get to any time
> > soon.
> > 
> >> Also, please make sure that the (mostly acked, but not yet commited) 
> >> changes for parser/Makefile also get applied to binutils/Makefile to keep 
> >> the difference small.
> >>
> >>> +.PHONY: install-rhel4
> >>> +install-rhel4: install-redhat
> >>> +
> >>> +.PHONY: install-redhat
> >>> +install-redhat:
> >>> +
> >>> +.PHONY: install-suse
> >>> +install-suse:
> >>> +
> >>> +.PHONY: install-slackware
> >>> +install-slackware:
> >>> +
> >>> +.PHONY: install-debian
> >>> +install-debian:
> >>> +
> >>> +.PHONY: install-unknown
> >>> +install-unknown:
> >>> +
> >>> +INSTALLDEPS=arch
> >>> +
> >>> +ifndef DISTRO
> >>> +DISTRO=$(shell if [ -f /etc/slackware-version ] ; then \
> >>> +	         echo slackware ; \
> >>> +	       elif [ -f /etc/debian_version ] ; then \
> >>> +	         echo debian ;\
> >>> +	       elif which rpm > /dev/null ; then \
> >>> +	         if [ "$(rpm --eval '0%{?suse_version}')" != "0" ] ; then \
> >>> +	             echo suse ;\
> >>> +	         elif [ "$(rpm --eval '%{_host_vendor}')" = redhat ] ; then
> >>> \ +	            echo rhel4 ;\
> >>> +	         elif [ "$(rpm --eval '0%{?fedora}')" != "0" ] ; then \
> >>> +	            echo rhel4 ;\
> >>> +	         else \
> >>> +	            echo unknown ;\
> >>> +	         fi ;\
> >>> +	       else \
> >>> +	          echo unknown ;\
> >>> +	       fi)
> >>> +endif
> >>> +
> >>> +ifdef DISTRO
> >>> +INSTALLDEPS+=install-$(DISTRO)
> >>> +endif
> >>
> >> All the install-$distribution (and also the distro detection) seems to 
> >> be unused. Do we really need it? 
> > 
> > Probably not. Maybe John can shed some light on why this was included?
> 
> cargo culting, I started with the parser Makefile and didn't spend the
> time to look into this any more

Ok, instead of revising this patch once more, I'm going to commit it
as-is (with all the other changes cboltz pointed out) and then submit a
follow up patch to remove the Makefile cruft.

> > 
> >> Note that we should probably keep "INSTALLDEPS=arch".
> >> (Again, this can be done in a follow-up patch.)
> >>
> >>> +clean: pod_clean
> >>> +	rm -f core core.* *.o *.s *.a *~ *.gcda *.gcno
> >>> +	rm -f gmon.out
> >>> +	rm -f $(TOOLS) $(TESTS)
> >>> +	rm -f $(NAME)*.tar.gz $(NAME)*.tgz
> >>
> >> Show me where this tarball gets built, and I'll accept that we need to 
> >> delete it ;-)  (I'd guess this sniplet is from the good old days when 
> >> you [1] still had separate tarballs for profiles, utils, parser etc.)
> > 
> > I will remove that line in my local copy of this commit.
> > 
> yeah vestiges of separate tar balls
> 
> >>> diff --git a/binutils/aa-enabled.pod b/binutils/aa-enabled.pod
> >>> new file mode 100644
> >>> index 0000000..217bc65
> >>> --- /dev/null
> >>> +++ b/binutils/aa-enabled.pod
> >>
> >>> +=head1 DESCRIPTION
> >>> +
> >>> +B<aa-enabled> is used to determine if AppArmor is enabled and
> >>> enforcing +policy.
> >>
> >> With the (in comparison to aa-status --enabled) removal of the check if 
> >> at least one profile is loaded, the "and enforcing policy" part looks a 
> >> bit confusing to me.
> > 
> > Agreed. I'll remove the "and enforcing policy" portion of that line.
> > 
> >>
> >> Removing that check is ok, I just want to be sure nobody misunderstands 
> >> what aa-enabled does. The release notes should also make that clear, and 
> >> explain that aa-enabled mostly replaces aa-status --enabled, but will 
> >> return 0 even if no profiles are loaded (which is probably an improvement 
> >> when used in package scripts).
> >>
> >>> +=head1 EXIT STATUS
> >> ...
> >>> +=item 2:
> >>> +
> >>> +intentionally not used as an B<aa-enabled> exit status.
> >>
> >> I wonder if we should explain the reason for this (see above).
> > 
> > Meh. A user isn't going to care outside of knowing that it isn't a
> > possible exit status.
> > 
> >>
> >>> diff --git a/binutils/aa_enabled.c b/binutils/aa_enabled.c
> >>> new file mode 100644
> >>> index 0000000..3ca84b0
> >>> --- /dev/null
> >>> +++ b/binutils/aa_enabled.c
> >>
> >> Looks good - but I'll let acking to someone who understands C better ;-)
> > 
> > Thanks for the review!
> > 
> > John wrote the patch. I've looked closely over all of the patch and you
> > have too. I think we have enough review to commit the patch once we hear
> > back from John regarding the install targets in the Makefile.
> > 
> sure, you can have my Acked-by for your bits :)

Thanks!

Tyler
-------------- 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/20151216/e59bf09f/attachment-0001.pgp>