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

[George Dunlap <george.dunlap@eu.citrix.com>]

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 (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.

>
>>>> Signed-off-by: Joby Poriyath <joby.poriyath@citrix.com>
>>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: george.dunlap@citrix.com
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> 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