Re: [osstest test] 56922: regressions - FAIL

[Robert Hu <robert.hu@vmm.sh.intel.com>]

On Sat, 2015-05-23 at 16:31 +0800, Robert Hu wrote:
> On Fri, 2015-05-22 at 15:21 +0100, Ian Campbell wrote:
> > On Fri, 2015-05-22 at 14:42 +0100, Ian Campbell wrote:
> > > From my particular grub.cfg. For real usage setupboot_grub2 will
> > > obviously need to become cleverer to count things correctly.
> > 
> > I've not tested extensively but the following incremental patch seems to
> > do the right thing, at least by inspection of the resulting grub.cfg.
> > 
> > Needs more testing (e.g. I haven't tried non-XSM yet) and review from
> > Ian I think, since there may be a more idiomatically Perl way to
> > manipulate the @offsets array (in particular shrinking it).
Carefully read your code and I think they can handle submenu (even
submenu in a submenu) correctly. Thanks for correcting my previous
implementation.
Just my 2 cents below inline.
> > 
> > Ian.
> > 
> > diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> > index 282175b..b5148fd 100644
> > --- a/Osstest/Debian.pm
> > +++ b/Osstest/Debian.pm
> > @@ -393,8 +393,6 @@ sub setupboot_grub1 ($$$$) {
> >  # Note on running OSSTest on Squeeze with old Xen kernel: check out
> >  # Debian bug #633127 "/etc/grub/20_linux does not recognise some old
> >  # Xen kernels"
> > -# Currently setupboot_grub2 relies on Grub menu not having submenu.
> > -# Check Debian bug #690538.
> >  sub setupboot_grub2 ($$$$) {
> >      my ($ho,$want_kernver,$want_xsm,$xenhopt,$xenkopt) = @_;
> >      my $bl= { };
> > @@ -405,7 +403,7 @@ sub setupboot_grub2 ($$$$) {
> >      my $parsemenu= sub {
> >          my $f= bl_getmenu_open($ho, $rmenu, "$stash/$ho->{Name}--grub.cfg.1");
> >      
> > -        my $count= 0;
> > +        my @offsets = (0);
> >          my $entry;
> >          my $submenu;
> >          while (<$f>) {
> > @@ -417,6 +415,8 @@ sub setupboot_grub2 ($$$$) {
> >                          "$submenu->{StartLine}. ".
> >                          "Our want kern is $want_kernver");
> >                      $submenu=undef;
> > +                    $#offsets = $#offsets-1;
> > +                    $offsets[$#offsets]++;
may consider 'pop/push' operations on @offsets array? I worry about if
'$#offsets-1' can always shrink array size correctly. pop/push anyway is
some official way to do this.
> >                      next;
> >                  }
> >                  my (@missing) =
> > @@ -446,11 +446,12 @@ sub setupboot_grub2 ($$$$) {
> >              }
> >              if (m/^menuentry\s+[\'\"](.*)[\'\"].*\{\s*$/) {
> >                  die $entry->{StartLine} if $entry;
> > -                $entry= { Title => $1, StartLine => $., Number => $count };
> > -                $count++;
> > +                $entry= { Title => $1, StartLine => $., MenuEntryPath => join ">", @offsets };
> > +                $offsets[$#offsets]++;
> >              }
> >              if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
> > -                $submenu={ StartLine =>$.};
> > +                $submenu={ StartLine =>$., MenuEntryPath => join ">", @offsets };
Here MenuEntryPath element for $submenu is actually debug purpose only,
I think, may be can remove it. correct me if I'm wrong.
> > +                $offsets[$#offsets+1] = 0;
> I see your points now.
> Seems you even considers nested submenu in the future.:)
> Just 1 question (I'm not a Perl expert): after "$offsets[$#offsets+1] =
> 0;", $#offsets increases automatically/implicitly, right?
> >              }
> >              if (m/^\s*multiboot\s*(?:\/boot)?\/(xen\S+)/) {
> >                  die unless $entry;
> > @@ -511,7 +512,7 @@ sub setupboot_grub2 ($$$$) {
> >              }
> >              print ::EO <<END or die $!;
> >  
> > -GRUB_DEFAULT=$entry->{Number}
> > +GRUB_DEFAULT="$entry->{MenuEntryPath}"
> >  END
> >  
> >              print ::EO <<END or die $! if defined $xenhopt;
> > 
> > 
> 
>