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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Oct 10, 2014 at 04:51:12PM +0100, Ian Jackson wrote:
> 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.
> 

No problem.

> > +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.
> 

OK.

> 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.
> 

OK, I missed the overlay file. No wonder the patch I wrote for my dev
machine did work on live OSSTest host!

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

No, I haven't. My plan was to file a bug after this series is merged.

But it looks like you would like me to file a bug first. I can do this
as well.

> > +   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.)
> 

If I'm to change the overlay file there won't be need to patch it
anymore.

Wei.

> Thanks,
> Ian.

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