[apparmor] [PATCH] parser: Add make variable to build against local or system libapparmor

Wed Dec 11 02:04:23 UTC 2013

On 12/06/2013 08:57 PM, 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
> 
> 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.
> 
> 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
> to the apparmor_parser target when building against the in-tree
> apparmor.
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>

I'm not a fan of sticking the
  include "apparmor.h"
or
  include <apparmor.h>

in the Makefile, but I can understand why you did it.

That said the patch looks good, I've gone through and tested it in the
various scenarios it I Love the feedback

Acked-by: John Johansen <john.johansen at canonical.com>

> ---
> 
> 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.
> 
>  parser/Makefile       | 48 ++++++++++++++++++++++++++++++++++++++----------
>  parser/dbus.c         |  2 +-
>  parser/parser_main.c  |  2 +-
>  parser/parser_misc.c  |  2 +-
>  parser/parser_regex.c |  2 +-
>  parser/parser_yacc.y  |  2 +-
>  6 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/parser/Makefile b/parser/Makefile
> index 20abd9e..8c5942d 100644
> --- a/parser/Makefile
> +++ b/parser/Makefile
> @@ -56,9 +56,7 @@ CFLAGS = -g -pg -fprofile-arcs -ftest-coverage
>  endif
>  endif #CFLAGS
>  
> -LIBAPPARMOR_PATH=../libraries/libapparmor/src/
> -LIBAPPARMOR_LDPATH=$(LIBAPPARMOR_PATH)/.libs/
> -EXTRA_CXXFLAGS = ${CFLAGS} ${CXX_WARNINGS} -std=gnu++0x -D_GNU_SOURCE -I$(LIBAPPARMOR_PATH)
> +EXTRA_CXXFLAGS = ${CFLAGS} ${CXX_WARNINGS} -std=gnu++0x -D_GNU_SOURCE
>  EXTRA_CFLAGS = ${EXTRA_CXXFLAGS} ${CPP_WARNINGS}
>  
>  #LEXLIB	:= -lfl
> @@ -90,9 +88,24 @@ OBJECTS = $(SRCS:.c=.o)
>  AAREDIR= libapparmor_re
>  AAREOBJECT = ${AAREDIR}/libapparmor_re.a
>  AAREOBJECTS = $(AAREOBJECT)
> -AARE_LDFLAGS=-static-libgcc -static-libstdc++ -L. -L$(LIBAPPARMOR_LDPATH)
> +AARE_LDFLAGS = -static-libgcc -static-libstdc++ -L.
>  AALIB = -Wl,-Bstatic -lapparmor -Wl,-Bdynamic -lpthread
>  
> +ifdef SYSTEM_LIBAPPARMOR
> +  # Using the system libapparmor so Makefile dependencies can't be used
> +  APPARMOR_H =
> +  LIBAPPARMOR_A =
> +  INCLUDE_APPARMOR_H = -include /usr/include/sys/apparmor.h
> +else
> +  LOCAL_LIBAPPARMOR_PATH = ../libraries/libapparmor/src
> +  LOCAL_LIBAPPARMOR_LDPATH = $(LOCAL_LIBAPPARMOR_PATH)/.libs
> +
> +  APPARMOR_H = $(LOCAL_LIBAPPARMOR_PATH)/apparmor.h
> +  LIBAPPARMOR_A = $(LOCAL_LIBAPPARMOR_LDPATH)/libapparmor.a
> +  INCLUDE_APPARMOR_H = -include $(APPARMOR_H)
> +  AARE_LDFLAGS += -L$(LOCAL_LIBAPPARMOR_LDPATH)
> +endif
> +
>  LEX_C_FILES	= parser_lex.c
>  YACC_C_FILES	= parser_yacc.c parser_yacc.h
>  
> @@ -156,7 +169,17 @@ all:	arch indep
>  coverage:
>  	$(MAKE) clean apparmor_parser COVERAGE=1
>  
> -apparmor_parser: $(OBJECTS) $(AAREOBJECTS)
> +ifndef SYSTEM_LIBAPPARMOR
> +$(LIBAPPARMOR_A):
> +	@if [ ! -f $@ ]; then \
> +		echo "error: $@ is missing. Pick one of these possible solutions:" 1>&2; \
> +		echo "  1) Build against the in-tree libapparmor by building it first and then trying again. See the top-level README for help." 1>&2; \
> +		echo "  2) Build against the system libapparmor by adding SYSTEM_LIBAPPARMOR=1 to your make command." 1>&2;\
> +		return 1; \
> +	fi
> +endif
> +
> +apparmor_parser: $(OBJECTS) $(AAREOBJECTS) $(LIBAPPARMOR_A)
>  	$(CXX) $(LDFLAGS) $(EXTRA_CFLAGS) -o $@ $(OBJECTS) $(LIBS) \
>  	      ${LEXLIB}  $(AAREOBJECTS) $(AARE_LDFLAGS) $(AALIB)
>  
> @@ -169,13 +192,16 @@ parser_lex.c: parser_lex.l parser_yacc.h parser.h profile.h
>  parser_lex.o: parser_lex.c parser.h parser_yacc.h
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
> -parser_misc.o: parser_misc.c parser.h parser_yacc.h profile.h af_names.h cap_names.h
> +parser_misc.o: EXTRA_CXXFLAGS += $(INCLUDE_APPARMOR_H)
> +parser_misc.o: parser_misc.c parser.h parser_yacc.h profile.h af_names.h cap_names.h $(APPARMOR_H)
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
> -parser_yacc.o: parser_yacc.c parser_yacc.h
> +parser_yacc.o: EXTRA_CXXFLAGS += $(INCLUDE_APPARMOR_H)
> +parser_yacc.o: parser_yacc.c parser_yacc.h $(APPARMOR_H)
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
> -parser_main.o: parser_main.c parser.h parser_version.h libapparmor_re/apparmor_re.h
> +parser_main.o: EXTRA_CXXFLAGS += $(INCLUDE_APPARMOR_H)
> +parser_main.o: parser_main.c parser.h parser_version.h libapparmor_re/apparmor_re.h $(APPARMOR_H)
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
>  parser_interface.o: parser_interface.c parser.h profile.h libapparmor_re/apparmor_re.h
> @@ -187,7 +213,8 @@ parser_include.o: parser_include.c parser.h parser_include.h
>  parser_merge.o: parser_merge.c parser.h profile.h
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
> -parser_regex.o: parser_regex.c parser.h profile.h libapparmor_re/apparmor_re.h
> +parser_regex.o: EXTRA_CXXFLAGS += $(INCLUDE_APPARMOR_H)
> +parser_regex.o: parser_regex.c parser.h profile.h libapparmor_re/apparmor_re.h $(APPARMOR_H)
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
>  parser_symtab.o: parser_symtab.c parser.h
> @@ -211,7 +238,8 @@ mount.o: mount.c mount.h parser.h immunix.h
>  lib.o: lib.c lib.h parser.h
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
> -dbus.o: dbus.c dbus.h parser.h immunix.h parser_yacc.h
> +dbus.o: EXTRA_CXXFLAGS += $(INCLUDE_APPARMOR_H)
> +dbus.o: dbus.c dbus.h parser.h immunix.h parser_yacc.h $(APPARMOR_H)
>  	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
>  
>  profile.o: profile.cc profile.h parser.h
> diff --git a/parser/dbus.c b/parser/dbus.c
> index f5aaca2..bdda14c 100644
> --- a/parser/dbus.c
> +++ b/parser/dbus.c
> @@ -18,7 +18,7 @@
>  
>  #include <stdlib.h>
>  #include <string.h>
> -#include <apparmor.h>
> +/* apparmor.h is included by Makefile */
>  
>  #include "parser.h"
>  #include "profile.h"
> diff --git a/parser/parser_main.c b/parser/parser_main.c
> index b9cc27d..befcc6e 100644
> --- a/parser/parser_main.c
> +++ b/parser/parser_main.c
> @@ -41,7 +41,7 @@
>  #include <sys/sysctl.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> -#include <apparmor.h>
> +/* apparmor.h is included by Makefile */
>  
>  #include "lib.h"
>  #include "parser.h"
> diff --git a/parser/parser_misc.c b/parser/parser_misc.c
> index dfa2240..9f2b852 100644
> --- a/parser/parser_misc.c
> +++ b/parser/parser_misc.c
> @@ -37,7 +37,7 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> -#include <apparmor.h>
> +/* apparmor.h is included by Makefile */
>  
>  #include "parser.h"
>  #include "profile.h"
> diff --git a/parser/parser_regex.c b/parser/parser_regex.c
> index 9452d3f..c13bd29 100644
> --- a/parser/parser_regex.c
> +++ b/parser/parser_regex.c
> @@ -21,7 +21,7 @@
>  #include <string.h>
>  #include <libintl.h>
>  #include <linux/limits.h>
> -#include <apparmor.h>
> +/* apparmor.h is included by Makefile */
>  #define _(s) gettext(s)
>  
>  /* #define DEBUG */
> diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y
> index cba1e90..4c7e137 100644
> --- a/parser/parser_yacc.y
> +++ b/parser/parser_yacc.y
> @@ -27,7 +27,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <libintl.h>
> -#include <apparmor.h>
> +/* apparmor.h is included by Makefile */
>  #define _(s) gettext(s)
>  
>  /* #define DEBUG */
> 