| Message ID | 20210908111727.440265-1-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v3] xen: rework `checkpolicy` detection when using "randconfig" | expand |
On 08.09.2021 13:17, Anthony PERARD wrote: > --- a/Config.mk > +++ b/Config.mk > @@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y > build_id_linker := --build-id=sha1 > endif > > -ifndef XEN_HAS_CHECKPOLICY > - CHECKPOLICY ?= checkpolicy > - XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n) > - export XEN_HAS_CHECKPOLICY > -endif Is there a particular reason to go from XEN_HAS_CHECKPOLICY to ... > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname) > PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python) > export PYTHON ?= $(PYTHON_INTERPRETER) > > +export CHECKPOLICY ?= checkpolicy > + > export BASEDIR := $(CURDIR) > export XEN_ROOT := $(BASEDIR)/.. > > @@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS) > export CLANG_FLAGS > endif > > +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) ... HAS_CHECKPOLICY? As soon as things get put in the environment, I'm always suspecting possible name collisions ... > @@ -189,14 +193,24 @@ ifeq ($(config-build),y) > # *config targets only - make sure prerequisites are updated, and descend > # in tools/kconfig to make the *config target > > +# Create a file for KCONFIG_ALLCONFIG which depends on the environment. > +# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig > +filechk_kconfig_allconfig = \ > + $(if $(findstring n,$(HAS_CHECKPOLICY)),echo 'CONFIG_XSM_FLASK_POLICY=n';) \ > + $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG), :) Nit: It would be nice if you were consistent with the blanks after commas in $(if ...). Personally I'm also considering $(if ...)s the more difficult to follow the longer they are. Hence for the 2nd one I wonder whether $(if $(KCONFIG_ALLCONFIG),cat,:) $(KCONFIG_ALLCONFIG) wouldn't be easier to read. > + > + Nit: Please avoid double blank lines. > +.allconfig.tmp: FORCE > + set -e; { $(call filechk_kconfig_allconfig); } > $@ Is there a particular reason for the .tmp suffix? Jan
On Thu, Sep 16, 2021 at 05:34:00PM +0200, Jan Beulich wrote: > On 08.09.2021 13:17, Anthony PERARD wrote: > > --- a/Config.mk > > +++ b/Config.mk > > @@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y > > build_id_linker := --build-id=sha1 > > endif > > > > -ifndef XEN_HAS_CHECKPOLICY > > - CHECKPOLICY ?= checkpolicy > > - XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n) > > - export XEN_HAS_CHECKPOLICY > > -endif > > Is there a particular reason to go from XEN_HAS_CHECKPOLICY to ... > > > --- a/xen/Makefile > > +++ b/xen/Makefile > > @@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname) > > PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python) > > export PYTHON ?= $(PYTHON_INTERPRETER) > > > > +export CHECKPOLICY ?= checkpolicy > > + > > export BASEDIR := $(CURDIR) > > export XEN_ROOT := $(BASEDIR)/.. > > > > @@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS) > > export CLANG_FLAGS > > endif > > > > +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) > > ... HAS_CHECKPOLICY? As soon as things get put in the environment, Not really anymore, it's just left over from having put this in Kconfig in previous version of the patch. > I'm always suspecting possible name collisions ... Yes, it's probably better to keep the XEN_ prefix. > > @@ -189,14 +193,24 @@ ifeq ($(config-build),y) > > # *config targets only - make sure prerequisites are updated, and descend > > # in tools/kconfig to make the *config target > > > > +# Create a file for KCONFIG_ALLCONFIG which depends on the environment. > > +# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig > > +filechk_kconfig_allconfig = \ > > + $(if $(findstring n,$(HAS_CHECKPOLICY)),echo 'CONFIG_XSM_FLASK_POLICY=n';) \ > > + $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG), :) > > Nit: It would be nice if you were consistent with the blanks after > commas in $(if ...). Personally I'm also considering $(if ...)s the > more difficult to follow the longer they are. Hence for the 2nd one > I wonder whether > > $(if $(KCONFIG_ALLCONFIG),cat,:) $(KCONFIG_ALLCONFIG) > > wouldn't be easier to read. How about: $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG);) \ : .. instead, as that would be more consistent with the previous line, that is there would be only one branch to the $(if ) and no else, and thus probably easier to read. > > +.allconfig.tmp: FORCE > > + set -e; { $(call filechk_kconfig_allconfig); } > $@ > > Is there a particular reason for the .tmp suffix? Yes, .*.tmp are already ignored via .gitignore. Thanks,
On 27.09.2021 11:46, Anthony PERARD wrote: > On Thu, Sep 16, 2021 at 05:34:00PM +0200, Jan Beulich wrote: >> On 08.09.2021 13:17, Anthony PERARD wrote: >>> @@ -189,14 +193,24 @@ ifeq ($(config-build),y) >>> # *config targets only - make sure prerequisites are updated, and descend >>> # in tools/kconfig to make the *config target >>> >>> +# Create a file for KCONFIG_ALLCONFIG which depends on the environment. >>> +# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig >>> +filechk_kconfig_allconfig = \ >>> + $(if $(findstring n,$(HAS_CHECKPOLICY)),echo 'CONFIG_XSM_FLASK_POLICY=n';) \ >>> + $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG), :) >> >> Nit: It would be nice if you were consistent with the blanks after >> commas in $(if ...). Personally I'm also considering $(if ...)s the >> more difficult to follow the longer they are. Hence for the 2nd one >> I wonder whether >> >> $(if $(KCONFIG_ALLCONFIG),cat,:) $(KCONFIG_ALLCONFIG) >> >> wouldn't be easier to read. > > How about: > > $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG);) \ > : > > .. instead, as that would be more consistent with the previous line, > that is there would be only one branch to the $(if ) and no else, and > thus probably easier to read. Oh, sure, even better if that works. >>> +.allconfig.tmp: FORCE >>> + set -e; { $(call filechk_kconfig_allconfig); } > $@ >> >> Is there a particular reason for the .tmp suffix? > > Yes, .*.tmp are already ignored via .gitignore. I see. Could you add two words to the description saying so? Or maybe even just a post-commit-message remark would do. Jan
diff --git a/Config.mk b/Config.mk index d0712724f8e4..144411133f38 100644 --- a/Config.mk +++ b/Config.mk @@ -137,12 +137,6 @@ export XEN_HAS_BUILD_ID=y build_id_linker := --build-id=sha1 endif -ifndef XEN_HAS_CHECKPOLICY - CHECKPOLICY ?= checkpolicy - XEN_HAS_CHECKPOLICY := $(shell $(CHECKPOLICY) -h 2>&1 | grep -q xen && echo y || echo n) - export XEN_HAS_CHECKPOLICY -endif - define buildmakevars2shellvars export PREFIX="$(prefix)"; \ export XEN_SCRIPT_DIR="$(XEN_SCRIPT_DIR)"; \ diff --git a/xen/Makefile b/xen/Makefile index f47423dacd9a..89804aefe385 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -17,6 +17,8 @@ export XEN_BUILD_HOST ?= $(shell hostname) PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 2>/dev/null) python) export PYTHON ?= $(PYTHON_INTERPRETER) +export CHECKPOLICY ?= checkpolicy + export BASEDIR := $(CURDIR) export XEN_ROOT := $(BASEDIR)/.. @@ -178,6 +180,8 @@ CFLAGS += $(CLANG_FLAGS) export CLANG_FLAGS endif +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen) + export root-make-done := y endif # root-make-done @@ -189,14 +193,24 @@ ifeq ($(config-build),y) # *config targets only - make sure prerequisites are updated, and descend # in tools/kconfig to make the *config target +# Create a file for KCONFIG_ALLCONFIG which depends on the environment. +# This will be use by kconfig targets allyesconfig/allmodconfig/allnoconfig/randconfig +filechk_kconfig_allconfig = \ + $(if $(findstring n,$(HAS_CHECKPOLICY)),echo 'CONFIG_XSM_FLASK_POLICY=n';) \ + $(if $(KCONFIG_ALLCONFIG), cat $(KCONFIG_ALLCONFIG), :) + + +.allconfig.tmp: FORCE + set -e; { $(call filechk_kconfig_allconfig); } > $@ + config: FORCE $(MAKE) $(kconfig) $@ # Config.mk tries to include .config file, don't try to remake it %/.config: ; -%config: FORCE - $(MAKE) $(kconfig) $@ +%config: .allconfig.tmp FORCE + $(MAKE) $(kconfig) KCONFIG_ALLCONFIG=$< $@ else # !config-build diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 0ddd18e11af3..73d8afb7bcbd 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -235,7 +235,7 @@ config XSM_FLASK_AVC_STATS config XSM_FLASK_POLICY bool "Compile Xen with a built-in FLASK security policy" - default y if "$(XEN_HAS_CHECKPOLICY)" = "y" + default y if "$(HAS_CHECKPOLICY)" depends on XSM_FLASK ---help--- This includes a default XSM policy in the hypervisor so that the
This will help prevent the CI loop from having build failures when `checkpolicy` isn't available when doing "randconfig" jobs. To prevent "randconfig" from selecting XSM_FLASK_POLICY when `checkpolicy` isn't available, we will actually override the config output with the use of KCONFIG_ALLCONFIG. Doing this way still allow a user/developer to set XSM_FLASK_POLICY even when "checkpolicy" isn't available. It also prevent the build system from reset the config when "checkpolicy" isn't available anymore. And XSM_FLASK_POLICY is still selected automatically when `checkpolicy` is available. But this also work well for "randconfig", as it will not select XSM_FLASK_POLICY when "checkpolicy" is missing. This patch allows to easily add more override which depends on the environment. Also, move the check out of Config.mk and into xen/ build system. Nothing in tools/ is using that information as it's done by ./configure. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- v3: - use KCONFIG_ALLCONFIG - don't override XSM_FLASK_POLICY value unless we do randconfig. - no more changes to the current behavior of kconfig, only to randconfig. v2 was "[XEN PATCH v2] xen: allow XSM_FLASK_POLICY only if checkpolicy binary is available" --- Config.mk | 6 ------ xen/Makefile | 18 ++++++++++++++++-- xen/common/Kconfig | 2 +- 3 files changed, 17 insertions(+), 9 deletions(-)