[PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Marcel J.E. Mol]

Booting a fedora 19 domU failed because a it could not properly
parse the grub.cfg file. This was cased by

	set default="${next_entry}"

This statement actually is within an 'if' statement, so maybe it would
be better to skip code within if/fi blocks...
But this patch seems to work fine.

Signed-off-by: Marcel Mol <marcel@mesa.nl>
---
 tools/pygrub/src/GrubConf.py |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
index 629951f..6324c62 100644
--- a/tools/pygrub/src/GrubConf.py
+++ b/tools/pygrub/src/GrubConf.py
@@ -427,6 +427,8 @@ class Grub2ConfigFile(_GrubConfigFile):
                 if self.commands[com] is not None:
                     if arg.strip() == "${saved_entry}":
                         arg = "0"
+                    elif arg.strip() == "${next_entry}":
+                        arg = "0"
                     setattr(self, self.commands[com], arg.strip())
                 else:
                     logging.info("Ignored directive %s" %(com,))
-- 
1.7.7.6

-- 
     ======--------         Marcel J.E. Mol                MESA Consulting B.V.
    =======---------        ph. +31-(0)6-54724868          P.O. Box 112
    =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
__==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
 They couldn't think of a number,           Linux user 1148  --  counter.li.org
    so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Ian Campbell]

On Mon, 2013-06-24 at 18:21 +0200, Marcel J.E. Mol wrote:
> Booting a fedora 19 domU failed because a it could not properly
> parse the grub.cfg file. This was cased by
> 
> 	set default="${next_entry}"
> 

Any chance you could send us an example of the actual problematic
configuration file, ideally as a patch adding a new file to
tools/pygrub/examples/. That "test suite" is rather rudimentary (to the
extent I'm a bit embarrassed to call it a suite...) but collecting them
when a bug is fixed is a good way to grow it.

> This statement actually is within an 'if' statement, so maybe it would
> be better to skip code within if/fi blocks...
> But this patch seems to work fine.
> 
> Signed-off-by: Marcel Mol <marcel@mesa.nl>

This change looks good to me:
Acked-by: Ian Campbell <ian.campbell@citix.com>

George, WRT 4.3 what do you think? I think the change is low risk for
any grub.conf without "${next_entry}" in it and a clear improvement to
any which does.

> ---
>  tools/pygrub/src/GrubConf.py |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
> index 629951f..6324c62 100644
> --- a/tools/pygrub/src/GrubConf.py
> +++ b/tools/pygrub/src/GrubConf.py
> @@ -427,6 +427,8 @@ class Grub2ConfigFile(_GrubConfigFile):
>                  if self.commands[com] is not None:
>                      if arg.strip() == "${saved_entry}":
>                          arg = "0"
> +                    elif arg.strip() == "${next_entry}":
> +                        arg = "0"
>                      setattr(self, self.commands[com], arg.strip())
>                  else:
>                      logging.info("Ignored directive %s" %(com,))
> -- 
> 1.7.7.6
> 

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Marcel J.E. Mol]

On Wed, Jun 26, 2013 at 02:50:46PM +0100, Ian Campbell wrote:
> On Mon, 2013-06-24 at 18:21 +0200, Marcel J.E. Mol wrote:
> > Booting a fedora 19 domU failed because a it could not properly
> > parse the grub.cfg file. This was cased by
> > 
> > 	set default="${next_entry}"
> > 
> 
> Any chance you could send us an example of the actual problematic
> configuration file, ideally as a patch adding a new file to
> tools/pygrub/examples/. That "test suite" is rather rudimentary (to the
> extent I'm a bit embarrassed to call it a suite...) but collecting them
> when a bug is fixed is a good way to grow it.

The grub.cfg is autogenerated in Fedora 19. I'll add a patch with the grub.cfg
that failed on me.

-- 
     ======--------         Marcel J.E. Mol                MESA Consulting B.V.
    =======---------        ph. +31-(0)6-54724868          P.O. Box 112
    =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
__==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
 They couldn't think of a number,           Linux user 1148  --  counter.li.org
    so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[George Dunlap]

On Mon, Jun 24, 2013 at 5:21 PM, Marcel J.E. Mol <marcel@mesa.nl> wrote:
> Booting a fedora 19 domU failed because a it could not properly
> parse the grub.cfg file. This was cased by
>
>         set default="${next_entry}"
>
> This statement actually is within an 'if' statement, so maybe it would
> be better to skip code within if/fi blocks...
> But this patch seems to work fine.
>
> Signed-off-by: Marcel Mol <marcel@mesa.nl>

Thanks Marcel -- is this in a default and/or auto-generated Fedora
grub.cfg, or was this something you put in manually?

In any case, I think it's a bit too late for 4.3 -- there's too much
risk that this might break something else; and there has been over two
months of RCs where Fedora users could have reported this bug.  It
will have to wait until 4.4 / 4.3.1.

If this is part of a default instal, however, we should probably add a
release note to that effect.

 -George

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Wei Liu]

On Wed, Jun 26, 2013 at 02:52:05PM +0100, George Dunlap wrote:
> On Mon, Jun 24, 2013 at 5:21 PM, Marcel J.E. Mol <marcel@mesa.nl> wrote:
> > Booting a fedora 19 domU failed because a it could not properly
> > parse the grub.cfg file. This was cased by
> >
> >         set default="${next_entry}"
> >
> > This statement actually is within an 'if' statement, so maybe it would
> > be better to skip code within if/fi blocks...
> > But this patch seems to work fine.
> >
> > Signed-off-by: Marcel Mol <marcel@mesa.nl>
> 
> Thanks Marcel -- is this in a default and/or auto-generated Fedora
> grub.cfg, or was this something you put in manually?
> 

I think this is auto-generated.

A patch back in May 3 changed grub.d/00_header.in.

   revno: 4969
   committer: Andrey Borzenkov <arvidjaar@gmail.com>
   branch nick: trunk
   timestamp: Mon 2013-05-06 22:13:34 +0400
   message:
         Reimplement grub-reboot to not depend on saved_entry. Use
         next_entry variable for one time boot menu entry.


What surprises me is that Fedora 19 picked up the change so quickly...


Wei.

> In any case, I think it's a bit too late for 4.3 -- there's too much
> risk that this might break something else; and there has been over two
> months of RCs where Fedora users could have reported this bug.  It
> will have to wait until 4.4 / 4.3.1.
> 

> If this is part of a default instal, however, we should probably add a
> release note to that effect.
> 
>  -George
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Marcel J.E. Mol]

On Wed, Jun 26, 2013 at 02:52:05PM +0100, George Dunlap wrote:
> On Mon, Jun 24, 2013 at 5:21 PM, Marcel J.E. Mol <marcel@mesa.nl> wrote:
> > Booting a fedora 19 domU failed because a it could not properly
> > parse the grub.cfg file. This was cased by
> >
> >         set default="${next_entry}"
> >
> > This statement actually is within an 'if' statement, so maybe it would
> > be better to skip code within if/fi blocks...
> > But this patch seems to work fine.
> >
> > Signed-off-by: Marcel Mol <marcel@mesa.nl>
> 
> Thanks Marcel -- is this in a default and/or auto-generated Fedora
> grub.cfg, or was this something you put in manually?

Yes, it is auto generated, with all the defaults installing a new
kernel will recreate the grub.cfg file.

> In any case, I think it's a bit too late for 4.3 -- there's too much
> risk that this might break something else; and there has been over two
> months of RCs where Fedora users could have reported this bug.  It
> will have to wait until 4.4 / 4.3.1.
> 
> If this is part of a default instal, however, we should probably add a
> release note to that effect.

It is quite a harmless patch. I wonder if it could break anything as
pygrub does not seem to evaluate $variable in a grub.cfg file
(correct me if I'm wrong...)

-Marcel
-- 
     ======--------         Marcel J.E. Mol                MESA Consulting B.V.
    =======---------        ph. +31-(0)6-54724868          P.O. Box 112
    =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
__==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
 They couldn't think of a number,           Linux user 1148  --  counter.li.org
    so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Konrad Rzeszutek Wilk]

On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
> Booting a fedora 19 domU failed because a it could not properly
> parse the grub.cfg file. This was cased by
> 
> 	set default="${next_entry}"
> 
> This statement actually is within an 'if' statement, so maybe it would
> be better to skip code within if/fi blocks...
> But this patch seems to work fine.
> 
> Signed-off-by: Marcel Mol <marcel@mesa.nl>

Excellent, and also

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

and it also fixes Red Hat bug: 978036

George, would you be OK putting this in 4.3?

> ---
>  tools/pygrub/src/GrubConf.py |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
> index 629951f..6324c62 100644
> --- a/tools/pygrub/src/GrubConf.py
> +++ b/tools/pygrub/src/GrubConf.py
> @@ -427,6 +427,8 @@ class Grub2ConfigFile(_GrubConfigFile):
>                  if self.commands[com] is not None:
>                      if arg.strip() == "${saved_entry}":
>                          arg = "0"
> +                    elif arg.strip() == "${next_entry}":
> +                        arg = "0"
>                      setattr(self, self.commands[com], arg.strip())
>                  else:
>                      logging.info("Ignored directive %s" %(com,))
> -- 
> 1.7.7.6
> 
> -- 
>      ======--------         Marcel J.E. Mol                MESA Consulting B.V.
>     =======---------        ph. +31-(0)6-54724868          P.O. Box 112
>     =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
> __==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
>  They couldn't think of a number,           Linux user 1148  --  counter.li.org
>     so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[George Dunlap]

On Wed, Jun 26, 2013 at 4:01 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
>> Booting a fedora 19 domU failed because a it could not properly
>> parse the grub.cfg file. This was cased by
>>
>>       set default="${next_entry}"
>>
>> This statement actually is within an 'if' statement, so maybe it would
>> be better to skip code within if/fi blocks...
>> But this patch seems to work fine.
>>
>> Signed-off-by: Marcel Mol <marcel@mesa.nl>
>
> Excellent, and also
>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> and it also fixes Red Hat bug: 978036
>
> George, would you be OK putting this in 4.3?

Hrm.  On further review, since this affects basically all Fedora
installations, and it mirrors a grub changeset that directlys witches
${saved_entry} to ${next_entry}, I guess on the whole it's worth the
risk.

Re the release:

Not-very-happilly-Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Ian Campbell]

On Wed, 2013-06-26 at 17:05 +0100, George Dunlap wrote:
> On Wed, Jun 26, 2013 at 4:01 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> > On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
> >> Booting a fedora 19 domU failed because a it could not properly
> >> parse the grub.cfg file. This was cased by
> >>
> >>       set default="${next_entry}"
> >>
> >> This statement actually is within an 'if' statement, so maybe it would
> >> be better to skip code within if/fi blocks...
> >> But this patch seems to work fine.
> >>
> >> Signed-off-by: Marcel Mol <marcel@mesa.nl>
> >
> > Excellent, and also
> >
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > and it also fixes Red Hat bug: 978036
> >
> > George, would you be OK putting this in 4.3?
> 
> Hrm.  On further review, since this affects basically all Fedora
> installations, and it mirrors a grub changeset that directlys witches
> ${saved_entry} to ${next_entry}, I guess on the whole it's worth the
> risk.
> 
> Re the release:
> 
> Not-very-happilly-Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Applied.

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)"):
> On Wed, 2013-06-26 at 17:05 +0100, George Dunlap wrote:
> > On Wed, Jun 26, 2013 at 4:01 PM, Konrad Rzeszutek Wilk
> > <konrad.wilk@oracle.com> wrote:
> > > On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
> > >> Booting a fedora 19 domU failed because a it could not properly
> > >> parse the grub.cfg file. This was cased by
> > >>
> > >>       set default="${next_entry}"
...
> Applied.

I have backported this to Xen 4.4, 4.3, 4.2, 4.1.  I went that far
back because effectively this is a compatibility fix to support new
guests, for which we need to have a long support lifetime.

Happily it applied without conflicts in each case.

Thanks,
Ian.

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Konrad Rzeszutek Wilk]

On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
> Booting a fedora 19 domU failed because a it could not properly
> parse the grub.cfg file. This was cased by
> 
> 	set default="${next_entry}"
> 
> This statement actually is within an 'if' statement, so maybe it would
> be better to skip code within if/fi blocks...
> But this patch seems to work fine.
> 
> Signed-off-by: Marcel Mol <marcel@mesa.nl>

Hey Marcel,

Thank you for finding the patch. As you have seen it is both in Xen 4.4 and
it is part of the Fedora 19 Xen 4.2 candidate patches. I was wondering if you
have the time to leave feedback at https://admin.fedoraproject.org/updates/FEDORA-2013-11837/xen-4.2.2-10.fc19
for the updated Xen RPMs which have this patch (and also some other ones)?

And again - thank you for taking the time to dig in PyGrub and to submit the
patch!

> ---
>  tools/pygrub/src/GrubConf.py |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
> index 629951f..6324c62 100644
> --- a/tools/pygrub/src/GrubConf.py
> +++ b/tools/pygrub/src/GrubConf.py
> @@ -427,6 +427,8 @@ class Grub2ConfigFile(_GrubConfigFile):
>                  if self.commands[com] is not None:
>                      if arg.strip() == "${saved_entry}":
>                          arg = "0"
> +                    elif arg.strip() == "${next_entry}":
> +                        arg = "0"
>                      setattr(self, self.commands[com], arg.strip())
>                  else:
>                      logging.info("Ignored directive %s" %(com,))
> -- 
> 1.7.7.6
> 
> -- 
>      ======--------         Marcel J.E. Mol                MESA Consulting B.V.
>     =======---------        ph. +31-(0)6-54724868          P.O. Box 112
>     =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
> __==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
>  They couldn't think of a number,           Linux user 1148  --  counter.li.org
>     so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH] pygrub/GrubConf: fix boot problem for fedora 19 grub.cfg (2nd attempt)

[Marcel J.E. Mol]

On Tue, Jul 02, 2013 at 11:01:18AM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Jun 24, 2013 at 06:21:32PM +0200, Marcel J.E. Mol wrote:
> > Booting a fedora 19 domU failed because a it could not properly
> > parse the grub.cfg file. This was cased by
> > 
> > 	set default="${next_entry}"
> > 
> > This statement actually is within an 'if' statement, so maybe it would
> > be better to skip code within if/fi blocks...
> > But this patch seems to work fine.
> > 
> > Signed-off-by: Marcel Mol <marcel@mesa.nl>
> 
> Hey Marcel,
> 
> Thank you for finding the patch. As you have seen it is both in Xen 4.4 and
> it is part of the Fedora 19 Xen 4.2 candidate patches. I was wondering if you
> have the time to leave feedback at https://admin.fedoraproject.org/updates/FEDORA-2013-11837/xen-4.2.2-10.fc19
> for the updated Xen RPMs which have this patch (and also some other ones)?

Done

> 
> And again - thank you for taking the time to dig in PyGrub and to submit the
> patch!
 
Glad to be of help. My server was still running the rawhide from
fedora core 8, being the latest dom0 fedora for a long time. I just
thought it was about time to update it :)  Fedora 19 was almost there
so I just gave it a try on a test machine. Running the old domU cliens
on the new xen server went fine and as I planned to also update the domU
clients, I first tried installing a fresh fedora 19 client. And we know what
happened then...
It was a bit difficult to debug the code as I'm not that fluent in python and
somehow the error message was vague as it does not indicate the real code
location where the problem occured.

Thanks,

-Marcel
-- 
     ======--------         Marcel J.E. Mol                MESA Consulting B.V.
    =======---------        ph. +31-(0)6-54724868          P.O. Box 112
    =======---------        marcel@mesa.nl                 2630 AC  Nootdorp
__==== www.mesa.nl ---____U_n_i_x______I_n_t_e_r_n_e_t____ The Netherlands ____
 They couldn't think of a number,           Linux user 1148  --  counter.li.org
    so they gave me a name!  -- Rupert Hine  --  www.ruperthine.com