Re: [OSSTEST Nested PATCH v11 2/7] Parsing grub which has 'submenu' primitive

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

On Wed, 2015-06-10 at 14:30 +0100, Ian Jackson wrote:
> longtao.pang writes ("[OSSTEST Nested PATCH v11 2/7] Parsing grub which has 'submenu' primitive"):
> > Now auto-gen kernel grub2 config file's boot menu entries can have
> > 2-level hierarchy, containing 'submenu' primitive, which is comprised by
> > several sub-menuentries. Xen boot entries are grouped into such kind of
> > 'submenu' block. This patch adds setupboot_grub2() ability to handle
> > such new grub.cfg format
> 
> Parsing submenus is good, but I have some style quibbles with your
> patch I'm afraid.
> 
> >          while (<$f>) {
> >              next if m/^\s*\#/ || !m/\S/;
> >              if (m/^\s*\}\s*$/) {
> > -                die unless $entry;
> > +                die unless $entry || $submenu;
> > +                if(!defined $entry && defined $submenu){
> 
> $entry and $submenu are either some ref, or undef, so you don't need
> `defined'.  You can just use them as if they were booleans in the if,
> just as you did above.  (I'm not sure that `!defined $entry' adds much
> given the previous line, but that's maybe a matter or taste.)
> 
> Missing spaces after if and inside `){'.
OK, to refine these.
> 
> > +                    logm("Met end of a submenu starting from ".
> > +                        "$submenu->{StartLine}. ".
> > +                        "Our want kern is $want_kernver");
> > +                    $submenu=undef;
> 
> Missing space after or around `='.  (Other places too.)
Wasn't aware of this rule. To refine this in next version patch.
> 
> > +                    pop @offsets;
> > +                    $offsets[$#offsets]++;
> > +                    next;
> > +                }
> >                  my (@missing) =
> >                      grep { !defined $entry->{$_} } 
> >  		        (defined $xenhopt
> > @@ -438,8 +448,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 };
> 
> Needs rewrapping.  Probably best to expand the { } into a multi-line
> structure.
OK, to divide it into 3 lines.
> 
> > +                $offsets[$#offsets]++;
> > +            }
> > +            if (m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/) {
> > +                $submenu={ StartLine =>$., MenuEntryPath => join ">", @offsets };
> 
> Unless I'm mistaken, the MenuEntryPath of a $submenu is never used ?
> Not setting it would avoid (a) a need to rewrap and (b) me complaining
> that you have open-coded the join twice.
Actually this contribution from Ian Campbell.
Hi Ian C., would you agree if I simply remove the 'MenuEntryPath' here? 
> 
> Thanks,
> Ian.