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?
> 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.
> > 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.
Tyler
>
>
> 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]
>
>
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
