* Re: [osstest test] 56922: regressions - FAIL
2015-05-22 14:21 ` Ian Campbell
@ 2015-05-23 3:35 ` Robert Hu
2015-05-23 6:46 ` Robert Hu
2015-05-23 8:31 ` Robert Hu
2015-05-23 13:34 ` Robert Hu
2 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-23 3:35 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
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).
Thanks Ian. You are so quick. I wrote that piece of code almost a year
ago; was just about to warm up.
We're to test your fix in our environment as well on non-XSM.
> 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]++;
> 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 };
> + $offsets[$#offsets+1] = 0;
> }
> 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;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 3:35 ` Robert Hu
@ 2015-05-23 6:46 ` Robert Hu
2015-05-23 6:58 ` Ian Campbell
0 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-23 6:46 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, Ian Campbell, xen-devel
On Sat, 2015-05-23 at 11:35 +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).
> Thanks Ian. You are so quick. I wrote that piece of code almost a year
> ago; was just about to warm up.
> We're to test your fix in our environment as well on non-XSM.
Just furbished up that piece of code. Now I recall the memory. This
piece of code was developed before 'XSM' things introduced; though now
after XSM things merged. May I know what is 'xsm' stuff?
Can you send me the 'grub.cfg' of those 'xsm' cases? so that I can take
a look.
> > 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]++;
> > 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 };
> > + $offsets[$#offsets+1] = 0;
> > }
> > 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;
> >
> >
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 6:46 ` Robert Hu
@ 2015-05-23 6:58 ` Ian Campbell
2015-05-23 7:52 ` Robert Hu
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2015-05-23 6:58 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, xen-devel
On Sat, 2015-05-23 at 14:46 +0800, Robert Hu wrote:
> On Sat, 2015-05-23 at 11:35 +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).
> > Thanks Ian. You are so quick. I wrote that piece of code almost a year
> > ago; was just about to warm up.
> > We're to test your fix in our environment as well on non-XSM.
>
> Just furbished up that piece of code. Now I recall the memory. This
> piece of code was developed before 'XSM' things introduced; though now
> after XSM things merged. May I know what is 'xsm' stuff?
XSM is Xen Security Modules (sort of selinux for Xen). It involves some
special additional entries in grub to provide the policy.
> Can you send me the 'grub.cfg' of those 'xsm' cases? so that I can take
> a look.
All of the logs for the flight can be found at:
http://logs.test-lab.xenproject.org/osstest/logs/56922/
Clicking the heading of one of the failing tests would take you to e.g.:
http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/info.html
and in there is:
http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/italia1--grub.cfg.1
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 6:58 ` Ian Campbell
@ 2015-05-23 7:52 ` Robert Hu
2015-05-23 8:25 ` Ian Campbell
0 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-23 7:52 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
On Sat, 2015-05-23 at 07:58 +0100, Ian Campbell wrote:
> On Sat, 2015-05-23 at 14:46 +0800, Robert Hu wrote:
> > On Sat, 2015-05-23 at 11:35 +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).
> > > Thanks Ian. You are so quick. I wrote that piece of code almost a year
> > > ago; was just about to warm up.
> > > We're to test your fix in our environment as well on non-XSM.
> >
> > Just furbished up that piece of code. Now I recall the memory. This
> > piece of code was developed before 'XSM' things introduced; though now
> > after XSM things merged. May I know what is 'xsm' stuff?
>
> XSM is Xen Security Modules (sort of selinux for Xen). It involves some
> special additional entries in grub to provide the policy.
>
> > Can you send me the 'grub.cfg' of those 'xsm' cases? so that I can take
> > a look.
>
> All of the logs for the flight can be found at:
>
> http://logs.test-lab.xenproject.org/osstest/logs/56922/
>
> Clicking the heading of one of the failing tests would take you to e.g.:
>
> http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/info.html
>From the log I see
'boot check: grub2, found Debian GNU/Linux, with Xen 4.6-unstable (XSM
enabled) and Linux 3.14.43+'
Seems it finds what ought to be.
>
> and in there is:
>
> http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/italia1--grub.cfg.1
>
> Ian.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 7:52 ` Robert Hu
@ 2015-05-23 8:25 ` Ian Campbell
2015-05-23 8:28 ` Robert Hu
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2015-05-23 8:25 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, xen-devel
On Sat, 2015-05-23 at 15:52 +0800, Robert Hu wrote:
> On Sat, 2015-05-23 at 07:58 +0100, Ian Campbell wrote:
> > On Sat, 2015-05-23 at 14:46 +0800, Robert Hu wrote:
> > > On Sat, 2015-05-23 at 11:35 +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).
> > > > Thanks Ian. You are so quick. I wrote that piece of code almost a year
> > > > ago; was just about to warm up.
> > > > We're to test your fix in our environment as well on non-XSM.
> > >
> > > Just furbished up that piece of code. Now I recall the memory. This
> > > piece of code was developed before 'XSM' things introduced; though now
> > > after XSM things merged. May I know what is 'xsm' stuff?
> >
> > XSM is Xen Security Modules (sort of selinux for Xen). It involves some
> > special additional entries in grub to provide the policy.
> >
> > > Can you send me the 'grub.cfg' of those 'xsm' cases? so that I can take
> > > a look.
> >
> > All of the logs for the flight can be found at:
> >
> > http://logs.test-lab.xenproject.org/osstest/logs/56922/
> >
> > Clicking the heading of one of the failing tests would take you to e.g.:
> >
> > http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/info.html
> From the log I see
> 'boot check: grub2, found Debian GNU/Linux, with Xen 4.6-unstable (XSM
> enabled) and Linux 3.14.43+'
> Seems it finds what ought to be.
Yes, but the corresponding thing which it puts into GRUB_DEFAULT (which
becomes "default" in grub.cfg) is "21" which is not correct, it needs to
be something like "8>1" to correctly refer to that entry.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 8:25 ` Ian Campbell
@ 2015-05-23 8:28 ` Robert Hu
0 siblings, 0 replies; 19+ messages in thread
From: Robert Hu @ 2015-05-23 8:28 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
On Sat, 2015-05-23 at 09:25 +0100, Ian Campbell wrote:
> On Sat, 2015-05-23 at 15:52 +0800, Robert Hu wrote:
> > On Sat, 2015-05-23 at 07:58 +0100, Ian Campbell wrote:
> > > On Sat, 2015-05-23 at 14:46 +0800, Robert Hu wrote:
> > > > On Sat, 2015-05-23 at 11:35 +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).
> > > > > Thanks Ian. You are so quick. I wrote that piece of code almost a year
> > > > > ago; was just about to warm up.
> > > > > We're to test your fix in our environment as well on non-XSM.
> > > >
> > > > Just furbished up that piece of code. Now I recall the memory. This
> > > > piece of code was developed before 'XSM' things introduced; though now
> > > > after XSM things merged. May I know what is 'xsm' stuff?
> > >
> > > XSM is Xen Security Modules (sort of selinux for Xen). It involves some
> > > special additional entries in grub to provide the policy.
> > >
> > > > Can you send me the 'grub.cfg' of those 'xsm' cases? so that I can take
> > > > a look.
> > >
> > > All of the logs for the flight can be found at:
> > >
> > > http://logs.test-lab.xenproject.org/osstest/logs/56922/
> > >
> > > Clicking the heading of one of the failing tests would take you to e.g.:
> > >
> > > http://logs.test-lab.xenproject.org/osstest/logs/56922/test-amd64-amd64-xl-xsm/info.html
> > From the log I see
> > 'boot check: grub2, found Debian GNU/Linux, with Xen 4.6-unstable (XSM
> > enabled) and Linux 3.14.43+'
> > Seems it finds what ought to be.
>
> Yes, but the corresponding thing which it puts into GRUB_DEFAULT (which
> becomes "default" in grub.cfg) is "21" which is not correct, it needs to
> be something like "8>1" to correctly refer to that entry.
Ah, I see.
>
> Ian.
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [osstest test] 56922: regressions - FAIL
2015-05-22 14:21 ` Ian Campbell
2015-05-23 3:35 ` Robert Hu
@ 2015-05-23 8:31 ` Robert Hu
2015-05-23 8:53 ` Robert Hu
2015-05-23 13:34 ` Robert Hu
2 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-23 8:31 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
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).
>
> 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]++;
> 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 };
> + $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;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 8:31 ` Robert Hu
@ 2015-05-23 8:53 ` Robert Hu
2015-05-24 13:53 ` Ian Campbell
0 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-23 8:53 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, Ian Campbell, xen-devel
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;
> >
> >
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-23 8:53 ` Robert Hu
@ 2015-05-24 13:53 ` Ian Campbell
2015-05-25 2:25 ` Robert Hu
0 siblings, 1 reply; 19+ messages in thread
From: Ian Campbell @ 2015-05-24 13:53 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, xen-devel
On Sat, 2015-05-23 at 16:53 +0800, Robert Hu wrote:
> > > + $#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.
Assigning to $#offsets is defined to change the length of the array. But
I think you are correct that pop/push would be a more idiomatic way to
do this.
It is a public holiday on Monday, but I'll take a look on Tuesday,
unless you fancy doing it in the meantime.
> @@ -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.
It is just for debug, I think it would be OK to leave it though.
Ian.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-24 13:53 ` Ian Campbell
@ 2015-05-25 2:25 ` Robert Hu
2015-05-25 3:04 ` Robert Hu
0 siblings, 1 reply; 19+ messages in thread
From: Robert Hu @ 2015-05-25 2:25 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
On Sun, 2015-05-24 at 14:53 +0100, Ian Campbell wrote:
> On Sat, 2015-05-23 at 16:53 +0800, Robert Hu wrote:
> > > > + $#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.
>
> Assigning to $#offsets is defined to change the length of the array. But
> I think you are correct that pop/push would be a more idiomatic way to
> do this.
>
> It is a public holiday on Monday, but I'll take a look on Tuesday,
> unless you fancy doing it in the meantime.
Sure I will do this.
>
> > @@ -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.
>
> It is just for debug, I think it would be OK to leave it though.
>
> Ian.
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [osstest test] 56922: regressions - FAIL
2015-05-25 2:25 ` Robert Hu
@ 2015-05-25 3:04 ` Robert Hu
0 siblings, 0 replies; 19+ messages in thread
From: Robert Hu @ 2015-05-25 3:04 UTC (permalink / raw)
To: robert.hu; +Cc: Pang, LongtaoX, ian.jackson, Ian Campbell, xen-devel
On Mon, 2015-05-25 at 10:25 +0800, Robert Hu wrote:
> On Sun, 2015-05-24 at 14:53 +0100, Ian Campbell wrote:
> > On Sat, 2015-05-23 at 16:53 +0800, Robert Hu wrote:
> > > > > + $#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.
> >
> > Assigning to $#offsets is defined to change the length of the array. But
> > I think you are correct that pop/push would be a more idiomatic way to
> > do this.
> >
> > It is a public holiday on Monday, but I'll take a look on Tuesday,
> > unless you fancy doing it in the meantime.
> Sure I will do this.
Done the test with pop/push alternative. Same effect as your original
patch.
Here is the trivial change on your patch.
diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 607c9ce..68101e0 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -415,7 +415,7 @@ sub setupboot_grub2 ($$$$) {
"$submenu->{StartLine}. ".
"Our want kern is $want_kernver");
$submenu=undef;
- $#offsets = $#offsets-1;
+ pop @offsets;
$offsets[$#offsets]++;
next;
}
@@ -451,7 +451,7 @@ sub setupboot_grub2 ($$$$) {
}
if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
$submenu={ StartLine =>$., MenuEntryPath => join ">",
@offsets };
- $offsets[$#offsets+1] = 0;
+ push @offsets,(0);
}
if (m/^\s*multiboot\s*(?:\/boot)?\/(xen\S+)/) {
die unless $entry;
> >
> > > @@ -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.
> >
> > It is just for debug, I think it would be OK to leave it though.
> >
> > Ian.
> >
>
>
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [osstest test] 56922: regressions - FAIL
2015-05-22 14:21 ` Ian Campbell
2015-05-23 3:35 ` Robert Hu
2015-05-23 8:31 ` Robert Hu
@ 2015-05-23 13:34 ` Robert Hu
2 siblings, 0 replies; 19+ messages in thread
From: Robert Hu @ 2015-05-23 13:34 UTC (permalink / raw)
To: Ian Campbell; +Cc: robert.hu, Pang, LongtaoX, ian.jackson, xen-devel
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).
I've done unit test with this piece of code, by making a sub comprised
of this piece of code and constructing various conditions: xsm, non-xsm,
xenhopt defined/undefined, all works as expected.
Of course, we'll further test this in real job-running cases.
>
> 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]++;
> 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 };
> + $offsets[$#offsets+1] = 0;
> }
> 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;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread