Re: [Xen-devel] [PATCH v4] xen/pygrub: grub2/grub.cfg from RHEL 7 has new commands in menuentry

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-02-05 at 12:33 +0000, George Dunlap wrote:
> On 02/05/2014 12:07 PM, Ian Campbell wrote:
> > On Wed, 2014-02-05 at 11:49 +0000, George Dunlap wrote:
> >> On 02/05/2014 09:22 AM, Ian Campbell wrote:
> >>> On Tue, 2014-02-04 at 18:10 +0000, Joby Poriyath wrote:
> >>>> menuentry in grub2/grub.cfg uses linux16 and initrd16 commands
> >>>> instead of linux and initrd. Due to this RHEL 7 (beta) guest failed to
> >>>> boot after the installation.
> >>>>
> >>>> In addition to this, RHEL 7 menu entries have two different single-quote
> >>>> delimited strings on the same line, and the greedy grouping for menuentry
> >>>> parsing gets both strings, and the options inbetween.
> >> So you're saying that adding the '?' just happens to change the match
> >> because of a quirk in the algorithms in the python library? That seems
> >> more like a hack than a proper fix; there may be other versions of
> >> python (future versions, for instance) where the new regexp will have
> >> the same effect as the old one, and we'll have another regression.
> >>
> >> Even if the behavior described is part of the defined interface,
> > I believe it is. Joby posted a link earlier. It also seems to be part of
> > the Perl re syntax -- and lots of things use Perl's regex syntax so I
> > think it is pretty "standard" (although I was not previously aware of it
> > either). Wikipedia's regex page talks about it too.
> >
> >>   I'd be
> >> wary of using this because future developers may not realize what it's
> >> for, or how to modify it properly to retain the properties it has now.
> > Hypothetical developer ignorance might call for a comment, but I think
> > avoiding language features which provide the semantics we need just
> > because they are a bit obscure would be a mistake.
> 
> Well of course having something fixed in an obscure fashion is better 
> than not having it fixed at all.  But having it fixed in a way which is 
> more obvious and doesn't rely on quirks of internal algorithms is even 
> better yet, if it can be done without being too ugly.  Wouldn't 
> something like the following work, and be more robust? (I haven't tested 
> this, I'm just going on Joby's description of the problem.)
> 
> title_match = re.match('^menuentry ["\']([^"\']*)["\'] (.*){', l)
> 
> 
> OTOH, if as Andrew claims 

As did I, right above.

> (in another e-mail in this thread), ".*?" is a 
> common idiom for exactly that (and since it's so common, I can well 
> believe there is an idiom) then it's even better to go with the idiom.

Yes, that was my point, this is apparently a common idiom.

> >>>> Signed-off-by: Joby Poriyath <joby.poriyath@xxxxxxxxxx>
> >>>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >>>> Cc: george.dunlap@xxxxxxxxxx
> >>> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>>
> >>> IMHO this can go into 4.4, unless George objects today I shall commit.
> >> I'm a bit on the fence about this one.  If this had been sent a month
> >> ago, it would be a no-brainer.  It certainly looks like it should work
> >> just fine.  On the other hand, pygrub is an important bit of
> >> functionality, and I'm not sure how much testing it gets. But of course
> >> the XenServer XenRT tests probably exercise it fairly well (or else they
> >> wouldn't be submitting this patch).
> > FWIW I intended to run it over the (admittedly small) set of test cases
> > in the tree as part of the commit process. I believe Joby has already
> > done so anyway.
> 
> Sure; I'm worried about the near infinite number of other grub configs 
> not in our tree. :-)  (Although the vast majority are likely to be 
> represented by those generated by a distro's update-grub with the 
> default parameters.)
> 
> On the whole, given that RHEL 7 is not yet out, and the timing of the 
> patch, I'm inclined to say this should wait until 4.4.1, unless there's 
> a reason that wouldn't be suitable.
> 
>   -George



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