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

[Christian Boltz]

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/

> 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.

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? 
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.)

> 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.

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).

> 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 ;-)


Regards,

Christian Boltz

[1] the switch to a single tarball happened before I started to work on
    AppArmor, therefore I'm not using "we" here ;-)
-- 
Es gibt einen alten chinesischen Fluch, Captain:
    Mögest Du in interessanteren Zeiten leben!
Inzwischen ist es höchst interessant, wenn Neelix in der Küche brutzelt.
["Harry Kim" in Star Trek: Voyager - Der mysteriöse Nebel]