This will help prevent the CI loop from having build failures when
`checkpolicy` isn't available, when doing "randconfig" jobs.

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>
---
We might want to have a new Makefile for this kind of check that
Kconfig is going to use, just to keep the main Makefile a bit cleaner.
But maybe another time, if more are comming.

v2:
- move check to Makefile
---
 Config.mk          | 6 ------
 xen/Makefile       | 4 ++++
 xen/common/Kconfig | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/Config.mk b/Config.mk
index d08fa8d60dd7..97d3633706b3 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 8023680ffbf2..045ddb18ad68 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)/..
 
@@ -156,6 +158,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
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 0ddd18e11af3..a5ef3814f531 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -235,8 +235,8 @@ 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"
-	depends on XSM_FLASK
+	default y
+	depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"
 	---help---
 	  This includes a default XSM policy in the hypervisor so that the
 	  bootloader does not need to load a policy to get sane behavior from an
-- 
Anthony PERARD

On 16.07.2021 14:38, Anthony PERARD wrote:
> This will help prevent the CI loop from having build failures when
> `checkpolicy` isn't available, when doing "randconfig" jobs.
> 
> 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>
> ---
> We might want to have a new Makefile for this kind of check that
> Kconfig is going to use, just to keep the main Makefile a bit cleaner.
> But maybe another time, if more are comming.
> 
> v2:
> - move check to Makefile

I'm afraid I don't understand:

> --- 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)/..
>  
> @@ -156,6 +158,8 @@ CFLAGS += $(CLANG_FLAGS)
>  export CLANG_FLAGS
>  endif
>  
> +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)

While the setting indeed gets obtained in a Makefile now, ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -235,8 +235,8 @@ 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"
> -	depends on XSM_FLASK
> +	default y
> +	depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"

... it's still used as a Kconfig dependency. This in particular
does not address George's concern about a setting silently getting
turned off behind the back of the person having enabled it (and
this could happen at any time, not just during the initial build,
where one might still remember to diff .config against
.config.old). The minimal thing imo is some kind of warning then.
Even better would be if the setting was left unchanged and the
build would fail; a solution for randconfig would then still be
needed of course. If we wanted to go this route, a tristate type
for the values may be unavoidable, along the lines of what Jürgen
has suggested. I wouldn't think of a global override though, but
a distinction of the origin of each option's setting. This might
be as simple as y vs Y for "positive" values and "# CONFIG_...
is not set" present (for visible options) or absent (for options
the user can't control) for "negative" ones. But yes, this would
likely be an intrusive change _and_ it would not be clear how to
transform existing .config-s, so is unlikely to be suitable for
the immediate issue at hand.

Jan

On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote:
> On 16.07.2021 14:38, Anthony PERARD wrote:
> > +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
> 
> While the setting indeed gets obtained in a Makefile now, ...
> 
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -235,8 +235,8 @@ 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"
> > -	depends on XSM_FLASK
> > +	default y
> > +	depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"
> 
> ... it's still used as a Kconfig dependency. This in particular
> does not address George's concern about a setting silently getting
> turned off behind the back of the person having enabled it (and

This patch v2 wasn't meant to address George's concern which didn't
exist at the time this v2 was sent... I was trying to address yours.

But it seems that "George's concern" is part of your issues with
Kconfig too, which I missed when trying to right this v2.

Anyway, those two patches are the only way I'm going to try to fix the
random build failure in the GitLab CI, I'm not going to try to fix
issues with the use of Kconfig for now. In the mean time either v1 or v2
is committed, or will just keep getting random build failure in the
GitLab CI.

-- 
Anthony PERARD

On 19.07.2021 12:47, Anthony PERARD wrote:
> On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote:
>> On 16.07.2021 14:38, Anthony PERARD wrote:
>>> +export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)
>>
>> While the setting indeed gets obtained in a Makefile now, ...
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -235,8 +235,8 @@ 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"
>>> -	depends on XSM_FLASK
>>> +	default y
>>> +	depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"
>>
>> ... it's still used as a Kconfig dependency. This in particular
>> does not address George's concern about a setting silently getting
>> turned off behind the back of the person having enabled it (and
> 
> This patch v2 wasn't meant to address George's concern which didn't
> exist at the time this v2 was sent... I was trying to address yours.
> 
> But it seems that "George's concern" is part of your issues with
> Kconfig too, which I missed when trying to right this v2.
> 
> Anyway, those two patches are the only way I'm going to try to fix the
> random build failure in the GitLab CI, I'm not going to try to fix
> issues with the use of Kconfig for now. In the mean time either v1 or v2
> is committed, or will just keep getting random build failure in the
> GitLab CI.

Fair enough. I actually think that randconfig shouldn't act quite as
randomly as it does. But what's sensible as behavior there really
depends heavily on the future intentions with .config. If we follow
Linux'es model (which Andrew advocates for), its randomness would be
limited by options which could get randomly set getting further
altered by environmental conditions. Hence that would limit what can
actually be tested, but it would avoid failures resulting from the
environment not matching the chose settings.

Otoh with our current model (largely, leaving aside the few
environment checks we've already got) what is being asked for is
what is going to get built. But failure from environmental
constraints shouldn't be treated the same as failure from bad
interaction of options; it's (aiui) the latter which randconfig is
supposed to point out.

Jan

On 19 Jul 2021, at 12:04, Jan Beulich <jbeulich@suse.com<mailto:jbeulich@suse.com>> wrote:

On 19.07.2021 12:47, Anthony PERARD wrote:
On Mon, Jul 19, 2021 at 09:37:06AM +0200, Jan Beulich wrote:
On 16.07.2021 14:38, Anthony PERARD wrote:
+export HAS_CHECKPOLICY := $(call success,$(CHECKPOLICY) -h 2>&1 | grep -q xen)

While the setting indeed gets obtained in a Makefile now, ...

--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -235,8 +235,8 @@ 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"
- depends on XSM_FLASK
+ default y
+ depends on XSM_FLASK && "$(HAS_CHECKPOLICY)"

... it's still used as a Kconfig dependency. This in particular
does not address George's concern about a setting silently getting
turned off behind the back of the person having enabled it (and

This patch v2 wasn't meant to address George's concern which didn't
exist at the time this v2 was sent... I was trying to address yours.

But it seems that "George's concern" is part of your issues with
Kconfig too, which I missed when trying to right this v2.

Anyway, those two patches are the only way I'm going to try to fix the
random build failure in the GitLab CI, I'm not going to try to fix
issues with the use of Kconfig for now. In the mean time either v1 or v2
is committed, or will just keep getting random build failure in the
GitLab CI.

Fair enough. I actually think that randconfig shouldn't act quite as
randomly as it does.

It sounds like randconfig currently works appropriately for how KConfig is meant to work.  That is, for good or for ill, KConfig seems designed to silently discard options with missing dependencies (either internal or external); so generating a config with missing dependencies to hand to it is “sensible”.


But what's sensible as behavior there really
depends heavily on the future intentions with .config. If we follow
Linux'es model (which Andrew advocates for), its randomness would be
limited by options which could get randomly set getting further
altered by environmental conditions. Hence that would limit what can
actually be tested, but it would avoid failures resulting from the
environment not matching the chose settings.

Otoh with our current model (largely, leaving aside the few
environment checks we've already got) what is being asked for is
what is going to get built. But failure from environmental
constraints shouldn't be treated the same as failure from bad
interaction of options; it's (aiui) the latter which randconfig is
supposed to point out.

Right; so another solution to the ‘randconfig CI loop’ issue would be to have the pipeline pass (or some non-pass non-fail state if possible) when then build failed because there were missing dependencies.  Maybe one way of doing that would be to have a “make check-dependencies” target that would run first.

That said, since it’s basically known that “silently disabling things it can’t build" is a quirk of Kconfig, I’m less inclined to object to v1 of the patch.

 -George

On 16/07/2021 13:38, Anthony PERARD wrote:
> This will help prevent the CI loop from having build failures when
> `checkpolicy` isn't available, when doing "randconfig" jobs.
>
> 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>
> ---
> We might want to have a new Makefile for this kind of check that
> Kconfig is going to use, just to keep the main Makefile a bit cleaner.
> But maybe another time, if more are comming.

I won't nack this patch, but I'm a very firm -2 against it.

v1 is the correct way to go.

~Andrew