From mboxrd@z Thu Jan  1 00:00:00 1970
From: Robert Hu <robert.hu@vmm.sh.intel.com>
Subject: Re: [OSSTEST Nested PATCH v11 2/7] Parsing grub which
 has 'submenu' primitive
Date: Thu, 11 Jun 2015 11:17:45 +0800
Message-ID: <1433992665.32731.6.camel@localhost>
References: <1432631304-27347-1-git-send-email-longtaox.pang@intel.com>
	<1432631304-27347-3-git-send-email-longtaox.pang@intel.com>
	<21880.15363.757545.925172@mariner.uk.xensource.com>
Reply-To: robert.hu@intel.com
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <21880.15363.757545.925172@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <Ian.Jackson@eu.citrix.com>, Ian.Campbell@citrix.com
Cc: robert.hu@intel.com, "longtao.pang" <longtaox.pang@intel.com>, wei.liu2@citrix.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.