[OSSTEST] enable FLASK_ENABLE when using it for testing
diff mbox

Message ID 1452783526-29173-1-git-send-email-cardoe@cardoe.com
State New, archived
Headers show

Commit Message

Doug Goldstein Jan. 14, 2016, 2:58 p.m. UTC
Currently OSSTest has 'XSM' tests but XSM and FLASK are two different
options and OSSTests's 'XSM' test depends on FLASK so ensure that FLASK
is enabled so that tests pass.

Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
 ts-xen-build | 1 +
 1 file changed, 1 insertion(+)

Comments

Ian Campbell Jan. 14, 2016, 4:22 p.m. UTC | #1
On Thu, 2016-01-14 at 08:58 -0600, Doug Goldstein wrote:
> Currently OSSTest has 'XSM' tests but XSM and FLASK are two different
> options and OSSTests's 'XSM' test depends on FLASK so ensure that FLASK
> is enabled so that tests pass.

IMHO the xen/Kconfig should be arranged such that if XSM is enabled then
FLASK is on by default. Having XSM but not Flask as what happens if you
enable XSM isn't helpful.

However, xen/common/Kconfig already has "default y" and "depends XSM" for
the FLASK entry, which I would have said was what was required to behave as
I suggest.

So why isn't it I wonder?

After a plain "make -C xen defconfig" I end up with a xen/.config
containing "CONFIG_XSM is not set" and no mention of CONFIG_FLASK (as
expected, due to the depends).

Adding CONFIG_XSM=y to the end of xen/.config and running "make -C xen
oldconfig" I then get asked:

    FLux Advanced Security Kernel support (FLASK) [Y/n/?] (NEW)  

IOW it does appear to be defaulting to on (I also get asked about
LATE_HWDOM).

osstest uses "olddefconfig", and with:

$ make -C xen olddefconfig
$ echo 'CONFIG_XSM=y' >> xen/.config  
$ make -C xen olddefconfig

I end up with both XSM and FLASK enabled:

$ grep -E XSM\|FLASK xen/.config
CONFIG_FLASK=y
CONFIG_XSM=y

which is what we want and is expected etc. Looking at the results of the
latest xen-unstable http://logs.test-lab.xenproject.org/osstest/logs/77945/
 and specifically at
http://logs.test-lab.xenproject.org/osstest/logs/77945/build-amd64-xsm/build/xen-hv-config
it seems that the automated version is doing what is expected too. IOW this
patch is both unnecessary (since it already works) and wrong (since it is
working how we want it to) and the issue exhibited by the test cases in
77945 is something different.

Ian.
Doug Goldstein Jan. 14, 2016, 4:34 p.m. UTC | #2
On 1/14/16 10:22 AM, Ian Campbell wrote:
> On Thu, 2016-01-14 at 08:58 -0600, Doug Goldstein wrote:
>> Currently OSSTest has 'XSM' tests but XSM and FLASK are two different
>> options and OSSTests's 'XSM' test depends on FLASK so ensure that FLASK
>> is enabled so that tests pass.
> 
> IMHO the xen/Kconfig should be arranged such that if XSM is enabled then
> FLASK is on by default. Having XSM but not Flask as what happens if you
> enable XSM isn't helpful.

That is how its setup. But a test framework should be explicit over
implicit over behaviors.

> 
> However, xen/common/Kconfig already has "default y" and "depends XSM" for
> the FLASK entry, which I would have said was what was required to behave as
> I suggest.
> 
> So why isn't it I wonder?

It is. If the test framework enabled FLASK since its testing FLASK then
I would agree with you. I'd actually argue that my patch didn't go far
enough and change all the places to FLASK instead of XSM.

> 
> After a plain "make -C xen defconfig" I end up with a xen/.config
> containing "CONFIG_XSM is not set" and no mention of CONFIG_FLASK (as
> expected, due to the depends).
> 
> Adding CONFIG_XSM=y to the end of xen/.config and running "make -C xen
> oldconfig" I then get asked:
> 
>     FLux Advanced Security Kernel support (FLASK) [Y/n/?] (NEW)  
> 
> IOW it does appear to be defaulting to on (I also get asked about
> LATE_HWDOM).
> 
> osstest uses "olddefconfig", and with:
> 
> $ make -C xen olddefconfig
> $ echo 'CONFIG_XSM=y' >> xen/.config  
> $ make -C xen olddefconfig
> 
> I end up with both XSM and FLASK enabled:
> 
> $ grep -E XSM\|FLASK xen/.config
> CONFIG_FLASK=y
> CONFIG_XSM=y
> 
> which is what we want and is expected etc. Looking at the results of the
> latest xen-unstable http://logs.test-lab.xenproject.org/osstest/logs/77945/
>  and specifically at
> http://logs.test-lab.xenproject.org/osstest/logs/77945/build-amd64-xsm/build/xen-hv-config
> it seems that the automated version is doing what is expected too. IOW this
> patch is both unnecessary (since it already works) and wrong (since it is
> working how we want it to) and the issue exhibited by the test cases in
> 77945 is something different.
> 
> Ian.
> 

This patch is for the tooling. The point of the patch is to be explicit
over implicit defaults. FLASK depends on XSM. The test framework tests
FLASK. It was relying on the behavior that FLASK is defaulted to enabled
when XSM is enabled but if it wants to test FLASK it should turn on FLASK.
Doug Goldstein Jan. 16, 2016, 8:54 p.m. UTC | #3
On 1/14/16 8:58 AM, Doug Goldstein wrote:
> Currently OSSTest has 'XSM' tests but XSM and FLASK are two different
> options and OSSTests's 'XSM' test depends on FLASK so ensure that FLASK
> is enabled so that tests pass.
> 
> Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
> ---
>  ts-xen-build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ts-xen-build b/ts-xen-build
> index 4812dff..228ceac 100755
> --- a/ts-xen-build
> +++ b/ts-xen-build
> @@ -60,6 +60,7 @@ END
>  		echo >>xen/.config CONFIG_FLASK='${build_xsm}'
>  	fi
>  	echo >>.config XSM_ENABLE='${build_xsm}'
> +	echo >>.config FLASK_ENABLE='${build_xsm}'
>  END
>                 (nonempty($r{tree_qemu}) ? <<END : '').
>  	echo >>.config QEMU_REMOTE='$r{tree_qemu}'
> 

Given the support of the maintainers and committers for the proposal
[1], this patch should NOT be applied.

[1]
http://lists.xenproject.org/archives/html/xen-devel/2016-01/msg01796.html

Patch
diff mbox

diff --git a/ts-xen-build b/ts-xen-build
index 4812dff..228ceac 100755
--- a/ts-xen-build
+++ b/ts-xen-build
@@ -60,6 +60,7 @@  END
 		echo >>xen/.config CONFIG_FLASK='${build_xsm}'
 	fi
 	echo >>.config XSM_ENABLE='${build_xsm}'
+	echo >>.config FLASK_ENABLE='${build_xsm}'
 END
                (nonempty($r{tree_qemu}) ? <<END : '').
 	echo >>.config QEMU_REMOTE='$r{tree_qemu}'