Re: [Xen-devel] [OSSTEST Nested PATCH v7 1/6] parsing grub which has 'submenu' primitive

["Hu, Robert" <robert.hu@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: Tuesday, March 31, 2015 9:44 PM
> To: Pang, LongtaoX
> Cc: xen-devel@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx; wei.liu2@xxxxxxxxxx;
> Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH v7 1/6] parsing grub which has 'submenu'
> primitive
> 
> On Fri, 2015-03-27 at 19:06 -0400, longtao.pang wrote:
> > From a hvm kernel build from Linux stable Kernel tree,
> > the auto generated grub2 menu will have 'submenu' primitive, upon the
> > 'menuentry' items. Xen boot entries will be grouped into a submenu. This
> > patch adds capability to support such grub formats.
> >
> > Signed-off-by: longtao.pang <longtaox.pang@xxxxxxxxx>
> > ---
> > Changes in v7:
> > Remove the reformatting change for Debian.pm and keep the original format.
> 
> Thank you.
> 
> > ---
> >  Osstest/Debian.pm |   21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> > index 6784024..35163a0 100644
> > --- a/Osstest/Debian.pm
> > +++ b/Osstest/Debian.pm
> > @@ -398,10 +398,18 @@ sub setupboot_grub2 ($$$$) {
> >
> >          my $count= 0;
> >          my $entry;
> > +        my $submenu;
> >          while (<$f>) {
> >              next if m/^\s*\#/ || !m/\S/;
> >              if (m/^\s*\}\s*$/) {
> > -                die unless $entry;
> > +                die unless $entry || $submenu;
> > +                if(!defined $entry && defined $submenu){
> > +                    logm("Met end of a submenu starting from ".
> > +                        "$submenu->{StartLine}. ".
> > +                        "Our want kern is $want_kernver");
> > +                    $submenu=undef;
> > +                    next;
> > +                }
> >                  my (@missing) =
> >                      grep { !defined $entry->{$_} }
> >                     (defined $xenhopt
> > @@ -432,21 +440,24 @@ sub setupboot_grub2 ($$$$) {
> >                  $entry= { Title => $1, StartLine => $., Number =>
> $count };
> >                  $count++;
> >              }
> > -            if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) {
> > +            if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
> > +                $submenu={ StartLine =>$.};
> > +            }
> 
> This looks reasonable enough to support a single nesting, I suppose we
> can leave more deeply nested submenus for another time.
> 
> So in that regard this patch looks ok to me.
> 
> > +            if (m/^\s*multiboot\s*(?:\/boot)*\/(xen\S+)/) {
> >                  die unless $entry;
> >                  $entry->{Hv}= $1;
> >              }
> > -            if (m/^\s*multiboot\s*\/(vmlinu[xz]-(\S+))/) {
> > +            if (m/^\s*multiboot\s*(?:\/boot)*\/(vmlinu[xz]-(\S+))/) {
> 
> What are these changes all about? I think they must be unrelated to the
> use of submenu (perhaps relate to having a separate /boot or not?). If
> so then please do in a separate patch.
> 
You're right. This has nothing to do with submenu.
Going to separate it out in another patch.
> If this is somehow to do with submenu then please explain how/why in the
> commit log.
> 
> BTW, your regex as it stand will accept /boot/boot/boot/boot/vmlinuz. I
> think you maybe meant to add "(?:\/boot)?" to match zero or one
> occurrences?
Yes, this is a potential bug. Thanks for point out!
> 
> Ian.

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