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
>
>> 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 :)
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
