On Tue, Dec 10, 2013 at 01:36:10PM -0800, Seth Arnold wrote: > Is building against the in-tree version the "best" default?
Yes. If there's been any development of the parser that depends on features of libapparmor that are newer than your system's libapparmor, then your compilation will fail. As a specific example, trunk's apparmor regression tests fail to compile on Ubuntu 12.04 LTS, because the query_label test program depends on the query interface added to libapparmor, which isn't present in 12.04's version of libapparmor. This will become more of an issue if we push some of the functionality currently in the parser into libapparmor, as is (long-term) planned. On Fri, Dec 06, 2013 at 08:57:57PM -0800, Tyler Hicks wrote: > By default, statically link against the in-tree libapparmor. If the > in-tree libapparmor is not yet built, print a helpful error message. > To build against the system libapparmor, the SYSTEM_LIBAPPARMOR make > variable can be set on the command line like so: > > $ make SYSTEM_LIBAPPARMOR=1 I suspect we also want a helpful message if SYSTEM_LIBAPPARMOR is set but the system libraries and/or header cannot be found, indicating that a libapparmor devel package needs to be installed. I am curious if John will complain about the length of the SYSTEM_LIBAPPARMOR variable, if he has to remember to define it on his make invocations. > This patch also fixes issues around the inclusion of the apparmor.h > header. Previously, the in-tree apparmor.h was always being included > even if the parser was being linked against the system libapparmor. > Parser source files should no longer include apparmor.h as the Makefile > includes the correct header at build time using the pre-processor's > -include option. > > This is fragile and definitely not ideal, but there is a valid reason > for doing it this way. -I../libraries/libapparmor/src/ was previously > used, but that is incorrect because there are header file collisions in > libraries/libapparmor/src/ and parser/. For example, a parser.h exists > in both directories. Ah, that's a very good point. I don't object to this approach for the parser. Another solution would be to move the public facing headers outside of the src/ tree (include/ ? include/sys/ ?) and add that location to the search path when building against in-tree libapparmor. That would also serve us should libapparmor grow additional external headers. One thing to be careful of is if the specific file needs included bits that are only available when _GNU_SOURCE is defined that happens to come from a header that also is included from libapparmor's apparmor.h; because the -include is treated as if it were the first thing in the compiled source file, _GNU_SOURCE would need to be defined on the compilation command line. > Per-target modification of EXTRA_CXXFLAGS is performed for source files > needing to include apparmor.h. Those targets were also updated to depend > on the local apparmor.h when building against the in-tree libapparmor. > When building against the system libapparmor, the variable used in the > dependency list is empty. Likewise, a libapparmor.la dependency is added I think you mean libapparmor.a dependency here (which is what is appropriate). > to the apparmor_parser target when building against the in-tree > apparmor. > > Signed-off-by: Tyler Hicks <[email protected]> > > This turned out being much uglier than I initially expected. This patch should > be fine to merge, but I'd love for someone to suggest something cleaner. I > particularly don't like including apparmor.h through a pre-processor option. The other approach that addresses the issue you pointed out would be to separate out the apparmor.h header out, as I mentioned above. And while I admit it's a bit ugly and has potential pitfalls around _GNU_SOURCE, it's not horrible. Anyway, Acked-by: Steve Beattie <[email protected]>. We can follow up with subsequent changes. I ended up, after trying different approaches, taking the same approach with compiling the apparmor regression tests; patch follows. I chose a different variable name "USE_SYSTEM" because I'd like to also decide to use which parser in the tests based on that flag (the in-tree version versus the system parser), with the ability to override with a user specified parser as well. The patch touches more files precisely because I hit the _GNU_SOURCE issue I mentioned above: certain files needed _GNU_SOURCE defined before the processing of headers that were included via apparmor.h, which led to warnings about the redefinition of both _GNU_SOURCE and _XOPEN_SOURCE (in particular, defining _GNU_SOURCE causes _XOPEN_SOURCE to be set to 700, not 500). Signed-off-by: Steve Beattie <[email protected]> --- tests/regression/apparmor/Makefile | 41 +++++++++++++++++++++----- tests/regression/apparmor/changehat.h | 1 tests/regression/apparmor/changehat_wrapper.c | 2 - tests/regression/apparmor/changeprofile.c | 1 tests/regression/apparmor/clone.c | 2 - tests/regression/apparmor/dbus_common.c | 3 + tests/regression/apparmor/dbus_eavesdrop.c | 3 + tests/regression/apparmor/dbus_message.c | 3 + tests/regression/apparmor/dbus_service.c | 3 + tests/regression/apparmor/env_check.c | 3 + tests/regression/apparmor/fchdir.c | 2 - tests/regression/apparmor/introspect.c | 2 - tests/regression/apparmor/onexec.c | 1 tests/regression/apparmor/openat.c | 2 - tests/regression/apparmor/pwrite.c | 3 + tests/regression/apparmor/query_label.c | 1 tests/regression/apparmor/unix_fd_client.c | 4 +- tests/regression/apparmor/unix_fd_server.c | 3 + 18 files changed, 54 insertions(+), 26 deletions(-) Index: b/tests/regression/apparmor/Makefile =================================================================== --- a/tests/regression/apparmor/Makefile +++ b/tests/regression/apparmor/Makefile @@ -1,10 +1,44 @@ # Copyright (C) 2002-2005 Novell/SUSE +# Copyright (C) 2013 Canonical, Ltd # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License as # published by the Free Software Foundation, version 2 of the # License. +ifdef USE_SYSTEM + # use the system libapparmor headers and library + LIBAPPARMOR = $(shell if pkg-config --exists libapparmor ; then \ + pkg-config --silence-errors --libs libapparmor ; \ + elif ldconfig -p | grep -q libapparmor\.so$$ ; then \ + echo -lapparmor ; \ + fi ) + ifeq ($(strip $(LIBAPPARMOR)),) + $(error Unable to find libapparmor installed on this echo system; either \ + install libapparmor devel packages, set the LIBAPPARMOR variable \ + manually, or build against in-tree libapparmor) + endif # LIBAPPARMOR not set + LIBAPPARMOR_H = /usr/include/sys/apparmor.h + LDLIBS += $(LIBAPPARMOR) + +else # !USE_SYSTEM + # use in-tree versions + LIBAPPARMOR_SRC := ../../../libraries/libapparmor/src/ + LIBAPPARMOR_H = $(LIBAPPARMOR_SRC)/apparmor.h + LIBAPPARMOR_PATH := $(LIBAPPARMOR_SRC)/.libs/ + ifeq ($(realpath $(LIBAPPARMOR_PATH)/libapparmor.a),) + $(error $(LIBAPPARMOR_PATH)/libapparmor.a is missing; either build against \ + the in-tree libapparmor by building it first and then trying again \ + (see the top-level README for help\) or build against the system \ + libapparmor by adding USE_SYSTEM=1 to your make command.) + endif + + CFLAGS += -L$(LIBAPPARMOR_PATH) + LDLIBS += -Wl,-Bstatic -lapparmor -Wl,-Bdynamic -lpthread +endif # USE_SYSTEM + +CFLAGS += -D_GNU_SOURCE -Wall -Wstrict-prototypes -include $(LIBAPPARMOR_H) + SRC=access.c \ introspect.c \ changeprofile.c \ @@ -82,13 +116,6 @@ ifneq (,$(findstring $(shell uname -i),i SRC+=syscall_ioperm.c syscall_iopl.c endif -LIBAPPARMOR:=$(shell if ldconfig -p | grep -q libapparmor\.so ; then \ - echo -lapparmor ; \ - fi ) - -CFLAGS+=-Wall -Wstrict-prototypes -LDLIBS+=$(LIBAPPARMOR) - EXEC=$(SRC:%.c=%) TESTS=access \ Index: b/tests/regression/apparmor/changehat.h =================================================================== --- a/tests/regression/apparmor/changehat.h +++ b/tests/regression/apparmor/changehat.h @@ -1,6 +1,5 @@ #include <fcntl.h> #include <string.h> -#include <sys/apparmor.h> #define SD_ID_MAGIC 0x8c235e38 Index: b/tests/regression/apparmor/changeprofile.c =================================================================== --- a/tests/regression/apparmor/changeprofile.c +++ b/tests/regression/apparmor/changeprofile.c @@ -17,7 +17,6 @@ #include <stdlib.h> #include <linux/unistd.h> -#include <sys/apparmor.h> #include "changehat.h" int main(int argc, char *argv[]) Index: b/tests/regression/apparmor/introspect.c =================================================================== --- a/tests/regression/apparmor/introspect.c +++ b/tests/regression/apparmor/introspect.c @@ -18,8 +18,6 @@ #include <stdlib.h> #include <linux/unistd.h> -#include <sys/apparmor.h> - int main(int argc, char *argv[]) { int rc; Index: b/tests/regression/apparmor/onexec.c =================================================================== --- a/tests/regression/apparmor/onexec.c +++ b/tests/regression/apparmor/onexec.c @@ -18,7 +18,6 @@ #include <signal.h> #include <linux/unistd.h> -#include <sys/apparmor.h> #include "changehat.h" int main(int argc, char *argv[]) Index: b/tests/regression/apparmor/query_label.c =================================================================== --- a/tests/regression/apparmor/query_label.c +++ b/tests/regression/apparmor/query_label.c @@ -1,7 +1,6 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <sys/apparmor.h> #define OPT_EXPECT "--expect=" #define OPT_EXPECT_LEN strlen(OPT_EXPECT) Index: b/tests/regression/apparmor/pwrite.c =================================================================== --- a/tests/regression/apparmor/pwrite.c +++ b/tests/regression/apparmor/pwrite.c @@ -9,7 +9,8 @@ * License. */ -#define _XOPEN_SOURCE 500 +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */ + #include <unistd.h> #include <sys/types.h> #include <sys/stat.h> Index: b/tests/regression/apparmor/changehat_wrapper.c =================================================================== --- a/tests/regression/apparmor/changehat_wrapper.c +++ b/tests/regression/apparmor/changehat_wrapper.c @@ -15,7 +15,7 @@ * inherited profile that is currently in a subprofile will stay that * way across fork()/exec(). */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ #include <stdio.h> #include <unistd.h> Index: b/tests/regression/apparmor/clone.c =================================================================== --- a/tests/regression/apparmor/clone.c +++ b/tests/regression/apparmor/clone.c @@ -7,7 +7,7 @@ * License. */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ #include <stdio.h> #include <unistd.h> Index: b/tests/regression/apparmor/dbus_common.c =================================================================== --- a/tests/regression/apparmor/dbus_common.c +++ b/tests/regression/apparmor/dbus_common.c @@ -22,7 +22,8 @@ * */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ + #include <stdio.h> #include <stdlib.h> #include <string.h> Index: b/tests/regression/apparmor/dbus_eavesdrop.c =================================================================== --- a/tests/regression/apparmor/dbus_eavesdrop.c +++ b/tests/regression/apparmor/dbus_eavesdrop.c @@ -23,7 +23,8 @@ * */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ + #include <stdio.h> #include <stdlib.h> #include <string.h> Index: b/tests/regression/apparmor/dbus_message.c =================================================================== --- a/tests/regression/apparmor/dbus_message.c +++ b/tests/regression/apparmor/dbus_message.c @@ -23,7 +23,8 @@ * */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ + #include <stdio.h> #include <stdlib.h> #include <string.h> Index: b/tests/regression/apparmor/fchdir.c =================================================================== --- a/tests/regression/apparmor/fchdir.c +++ b/tests/regression/apparmor/fchdir.c @@ -7,7 +7,7 @@ * License. */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ #include <stdio.h> #include <unistd.h> Index: b/tests/regression/apparmor/openat.c =================================================================== --- a/tests/regression/apparmor/openat.c +++ b/tests/regression/apparmor/openat.c @@ -7,7 +7,7 @@ * License. */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ #include <stdio.h> #include <stdlib.h> Index: b/tests/regression/apparmor/unix_fd_client.c =================================================================== --- a/tests/regression/apparmor/unix_fd_client.c +++ b/tests/regression/apparmor/unix_fd_client.c @@ -1,5 +1,3 @@ -#define _XOPEN_SOURCE 500 - /* * Copyright (C) 2002-2005 Novell/SUSE * @@ -9,6 +7,8 @@ * License. */ +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */ + #include <stdio.h> #include <unistd.h> #include <sys/types.h> Index: b/tests/regression/apparmor/unix_fd_server.c =================================================================== --- a/tests/regression/apparmor/unix_fd_server.c +++ b/tests/regression/apparmor/unix_fd_server.c @@ -12,7 +12,8 @@ /* this is very ugly */ -#define _XOPEN_SOURCE 500 +/* needs either _XOPEN_SOURCE or _GNU_SOURCE defined */ + #include <stdio.h> #include <unistd.h> #include <sys/types.h> Index: b/tests/regression/apparmor/dbus_service.c =================================================================== --- a/tests/regression/apparmor/dbus_service.c +++ b/tests/regression/apparmor/dbus_service.c @@ -23,7 +23,8 @@ * */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ + #include <stdio.h> #include <stdlib.h> #include <string.h> Index: b/tests/regression/apparmor/env_check.c =================================================================== --- a/tests/regression/apparmor/env_check.c +++ b/tests/regression/apparmor/env_check.c @@ -6,7 +6,8 @@ * License. */ -#define _GNU_SOURCE +/* needs _GNU_SOURCE defined */ + #include <stdio.h> #include <stdlib.h> #include <string.h> -- Steve Beattie <[email protected]> http://NxNW.org/~steve/
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
