[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [OSSTEST Nested PATCH v11 2/7] Parsing grub which has 'submenu' primitive
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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |