Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has 'submenu' primitive

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

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Thursday, February 12, 2015 6:13 PM
> To: Hu, Robert
> Cc: Ian Jackson; xen-devel@lists.xen.org; jfehlig@suse.com;
> wei.liu2@citrix.com; ian.campbell@citrix.com; Pang, LongtaoX
> Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> 'submenu' primitive
> 
> On Thu, Feb 12, 2015 at 02:01:59AM +0000, Hu, Robert wrote:
> >
> > > -----Original Message-----
> > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com]
> > > Sent: Wednesday, February 11, 2015 10:44 PM
> > > To: Hu, Robert
> > > Cc: xen-devel@lists.xen.org; jfehlig@suse.com; wei.liu2@citrix.com;
> > > ian.campbell@citrix.com; Pang, LongtaoX
> > > Subject: Re: [PATCH OSSTEST 01/12] Add support of parsing grub which has
> > > 'submenu' primitive
> > >
> > > Robert Ho writes ("[PATCH OSSTEST 01/12] Add support of parsing grub
> which
> > > has 'submenu' primitive"):
> > > >  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. Also, this patch
> adjust
> > > >  some indent alignments.
> > >
> > > Thanks for this submission.  Dealing with submenus is definitely
> > > something we want to do.
> > >
> > > I haven't looked at the code in detail yet but I have a general
> > > question: we currently count menu entries and eventually write
> > > GRUB_DEFAULT=<some number>  into /etc/default/grub.
> > >
> > > Does this work properly if the entry is in a submenu ?  I guess you
> > > have probably tested this but I thought I should ask...
> > >
> > Yes, this minor change just get 'parsemenu' subroutine capability of
> recognizing 'submenu'.
> > The outer layer logic isn't affected.
> > Actually, the Xen boot menuentry we choose, is inside a submenu. It works
> and /etc/default/grub
> > is assigned properly.
> 
> In any case this is a very useful improvement.
> 
> Out of interest what Linux are you running?  If you're running Debian
> and the overlay 20_linux_xen (inside $OSSTEST/overlay/etc/etc/grub.d) is
> copied to your test host, there shouldn't be any submenu entries in your
> grub.cfg, I think.
> 
> Wei.
We use Debian + linux-stable kernel in the test.
Didn't look into details of the grub generating procedure, but my observation
is that it does have the submenu.
> 
> > > Can you please not adjust the whitespace ?  osstest in general doesn't
> > > have a requirement for any particular whitespace use, and certainly if
> > > there are to be any whitespace changes they ought to be in a separate
> > > patch.
> > I adjust those because some one in last version's reply told us that
> > osstest prefers white space substitution to tab, and traditionally 4
> > white space of 1 tab. (This align with my previous coding experience as well)
> > And I indeed find that this hunk of code doesn't looks well in my editor.
> > Its unalignment increases difficulty of reading.
> > I would suggest to adjust the this hunk's indentation and use white space
> > substitution to tab to have best suitability across different editors.
> > >
> > > Thanks,
> > > Ian.