Re: [Xen-devel] [OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM support if requested

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("[OSSTEST PATCH v2 08/12] ts-xen-install: install Xen with XSM 
support if requested"):
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
>  ts-xen-install |  115 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/ts-xen-install b/ts-xen-install
> index 4d34d1f..3bc4d2e 100755
> --- a/ts-xen-install
> +++ b/ts-xen-install
> @@ -46,6 +46,8 @@ if (@ARGV and $ARGV[0] eq '--check') {
>  
>  our $ho;
>  
> +my $enable_xsm = $r{enable_xsm} =~ m/y/ ? 1 : 0;
> +
>  my %distpath;
>  
>  sub packages () {
> @@ -73,6 +75,15 @@ sub extract () {
>                                  $r{"${part}buildjob"}, \%distpath);
>      }
>      target_cmd_root($ho, '/sbin/ldconfig');
> +    if ($enable_xsm) {
> +        my $flaskpolicy = target_cmd_output_root($ho,
> +            'find /boot -name \'xenpolicy-*\' -exec basename {} \;');
> +     # there should only be one xenpolicy file for a clean install
> +     my $c = () = $flaskpolicy =~ /xenpolicy/g;
> +     die "Too many XSM policy files $c" if $c > 1;
> +     die "XSM policy file is required" if $c == 0;
> +     store_runvar("flaskpolicy", $flaskpolicy);

I don't much like this, I'm afraid.

I think this filename or version should be recorded by ts-xen-build
and the value used here.  Cf the `kernel_ver' runvar saved by
ts-kernel-build and used by ts-xen-install.

> +sub grub_patch () {
> +    return << 'END';

I'm afraid this isn't right.

Firstly, if we are patching this, the patch ought to be in a separate
file.

Secondly, overlay/etc/grub.d/20_linux_xen already exists (mentioning
Debian bug #633127).  This is automatically copied into the installed
system.  So any change should be made there.

Thirdly, you should mention the bug report that you have filed (you've
filed one, right?) about the need to patch in this support.

> +     target_cmd_root($ho, << 'END');
> +if test ! -e /etc/grub.d/20_linux_xen ; then
> +  case `uname -m` in
> +    x86*) echo '/etc/grub.d/20_linux_xen doesn't exist, abort'
> +          exit 1 ;;

Surely the right thing here is just to carry on.  Perhaps the file was
improved and renamed or something.  (If this it is to be done like
this at all, which I doubt - see above.)

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel