[PATCH] pygrub: fix non-interactive parsing of grub1 config files

[PATCH] pygrub: fix non-interactive parsing of grub1 config files

[David Scott]

From: David Scott <dave.scott@eu.citrix.com>

Since c/s d1b93ea2, the type of 'sel' now depends on whether pygrub
is run interactively or not. In non-interactive / quiet mode it is
a string; in interactive mode it is an integer.

When 'sel' is used to index an array it must be used as an integer.

Signed-off-by: David Scott <dave.scott@citrix.com>
---
 tools/pygrub/src/pygrub |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
index 2618e11..f167a51 100644
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -630,7 +630,7 @@ def run_grub(file, entry, fs, cfg_args):
         sys.exit(1)
 
     try:
-        img = g.cf.images[sel]
+        img = g.cf.images[int(sel)]
     except IndexError:
         img = g.cf.images[0]
 
-- 
1.7.10.4

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Boris Ostrovsky]

On 09/08/2014 10:01 AM, David Scott wrote:
> From: David Scott <dave.scott@eu.citrix.com>
>
> Since c/s d1b93ea2, the type of 'sel' now depends on whether pygrub
> is run interactively or not. In non-interactive / quiet mode it is
> a string; in interactive mode it is an integer.
>
> When 'sel' is used to index an array it must be used as an integer.
>
> Signed-off-by: David Scott <dave.scott@citrix.com>
> ---
>   tools/pygrub/src/pygrub |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index 2618e11..f167a51 100644
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -630,7 +630,7 @@ def run_grub(file, entry, fs, cfg_args):
>           sys.exit(1)
>   
>       try:
> -        img = g.cf.images[sel]
> +        img = g.cf.images[int(sel)]

What if sel is a "true" string (i.e. a name of the kernel to boot as 
opposed to an index)? Should we do the same thing as what Grub:run() 
does to map value to index?

-boris


>       except IndexError:
>           img = g.cf.images[0]
>   

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Dave Scott]

On 8 Sep 2014, at 18:11, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 09/08/2014 10:01 AM, David Scott wrote:
>> From: David Scott <dave.scott@eu.citrix.com>
>> 
>> Since c/s d1b93ea2, the type of 'sel' now depends on whether pygrub
>> is run interactively or not. In non-interactive / quiet mode it is
>> a string; in interactive mode it is an integer.
>> 
>> When 'sel' is used to index an array it must be used as an integer.
>> 
>> Signed-off-by: David Scott <dave.scott@citrix.com>
>> ---
>>  tools/pygrub/src/pygrub |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
>> index 2618e11..f167a51 100644
>> --- a/tools/pygrub/src/pygrub
>> +++ b/tools/pygrub/src/pygrub
>> @@ -630,7 +630,7 @@ def run_grub(file, entry, fs, cfg_args):
>>          sys.exit(1)
>>        try:
>> -        img = g.cf.images[sel]
>> +        img = g.cf.images[int(sel)]
> 
> What if sel is a "true" string (i.e. a name of the kernel to boot as opposed to an index)? Should we do the same thing as what Grub:run() does to map value to index?

Redefining ‘sel’ from an index into an array into to a key of a dictionary sounds plausible to me… but I’m not really familiar enough with pygrub to offer a trustworthy opinion.

I’ve cc:d Simon Rowe who is more familiar with pygrub — he may have some useful thoughts.

Dave
> 
> -boris
> 
> 
>>      except IndexError:
>>          img = g.cf.images[0]

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Konrad Rzeszutek Wilk]

On Wed, Sep 10, 2014 at 12:50:29PM +0000, Dave Scott wrote:
> 
> On 8 Sep 2014, at 18:11, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> 
> > On 09/08/2014 10:01 AM, David Scott wrote:
> >> From: David Scott <dave.scott@eu.citrix.com>
> >> 
> >> Since c/s d1b93ea2, the type of 'sel' now depends on whether pygrub
> >> is run interactively or not. In non-interactive / quiet mode it is
> >> a string; in interactive mode it is an integer.
> >> 
> >> When 'sel' is used to index an array it must be used as an integer.
> >> 
> >> Signed-off-by: David Scott <dave.scott@citrix.com>
> >> ---
> >>  tools/pygrub/src/pygrub |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> >> index 2618e11..f167a51 100644
> >> --- a/tools/pygrub/src/pygrub
> >> +++ b/tools/pygrub/src/pygrub
> >> @@ -630,7 +630,7 @@ def run_grub(file, entry, fs, cfg_args):
> >>          sys.exit(1)
> >>        try:
> >> -        img = g.cf.images[sel]
> >> +        img = g.cf.images[int(sel)]
> > 
> > What if sel is a "true" string (i.e. a name of the kernel to boot as opposed to an index)? Should we do the same thing as what Grub:run() does to map value to index?
> 
> Redefining ?sel? from an index into an array into to a key of a dictionary sounds plausible to me? but I?m not really familiar enough with pygrub to offer a trustworthy opinion.
> 
> I?ve cc:d Simon Rowe who is more familiar with pygrub ? he may have some useful thoughts.
>

ping ? 
> Dave
> > 
> > -boris
> > 
> > 
> >>      except IndexError:
> >>          img = g.cf.images[0]
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Simon Rowe]

On 24/10/14 22:15, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 10, 2014 at 12:50:29PM +0000, Dave Scott wrote:
>>
>> Redefining ?sel? from an index into an array into to a key of a dictionary sounds plausible to me? but I?m not really familiar enough with pygrub to offer a trustworthy opinion.
>>
>> I?ve cc:d Simon Rowe who is more familiar with pygrub ? he may have some useful thoughts.
>>
> ping ?
>

I think this changeset could just cause a ValueError to be thrown if a 
label was in cf.default at this point. I've got a patch I'm dev testing, 
I'll post it shortly,

Simon

[PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Simon Rowe]

Changes to handle non-numeric default attributes for grub2 caused run_grub()
to attempt to index into the images list using a string. Pull out the code
that handles submenus into a new function and use that to ensure sel is
numeric.

Reported-by: David Scott <dave.scott@citrix.com>
Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>
---
 tools/pygrub/src/pygrub |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
index 1ae34a2..4cc846f 100644
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -456,11 +456,9 @@ class Grub:
         del f
         self.cf.parse(buf)
 
-    def run(self):
-        timeout = int(self.cf.timeout)
-
+    def image_index(self):
         if self.cf.default.isdigit():
-            self.selected_image = int(self.cf.default)
+            sel = int(self.cf.default)
         else:
             # We don't fully support submenus. Look for the leaf value in
             # "submenu0>submenu1>...>menuentry" and hope that it's unique.
@@ -472,16 +470,23 @@ class Grub:
                     break
 
             # Map string to index in images array
-            self.selected_image = 0
+            sel = 0
             for i in range(len(self.cf.images)):
                 if self.cf.images[i].title == title:
-                    self.selected_image = i
+                    sel = i
                     break
 
         # If the selected (default) image doesn't exist we select the first entry
-        if self.selected_image > len(self.cf.images):
+        if sel > len(self.cf.images):
             logging.warning("Default image not found")
-            self.selected_image = 0
+            sel = 0
+
+        return sel
+
+    def run(self):
+        timeout = int(self.cf.timeout)
+        self.selected_image = self.image_index()
+
         self.isdone = False
         while not self.isdone:
             self.run_main(timeout)
@@ -626,7 +631,7 @@ def run_grub(file, entry, fs, cfg_args):
     if interactive and not list_entries:
         curses.wrapper(run_main)
     else:
-        sel = g.cf.default
+        sel = g.image_index()
 
     # set the entry to boot as requested
     if entry is not None:
-- 
1.7.10.4

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Boris Ostrovsky]

On 10/27/2014 12:00 PM, Simon Rowe wrote:
> Changes to handle non-numeric default attributes for grub2 caused run_grub()
> to attempt to index into the images list using a string. Pull out the code
> that handles submenus into a new function and use that to ensure sel is
> numeric.
>
> Reported-by: David Scott <dave.scott@citrix.com>
> Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

> ---
>   tools/pygrub/src/pygrub |   23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index 1ae34a2..4cc846f 100644
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -456,11 +456,9 @@ class Grub:
>           del f
>           self.cf.parse(buf)
>   
> -    def run(self):
> -        timeout = int(self.cf.timeout)
> -
> +    def image_index(self):
>           if self.cf.default.isdigit():
> -            self.selected_image = int(self.cf.default)
> +            sel = int(self.cf.default)
>           else:
>               # We don't fully support submenus. Look for the leaf value in
>               # "submenu0>submenu1>...>menuentry" and hope that it's unique.
> @@ -472,16 +470,23 @@ class Grub:
>                       break
>   
>               # Map string to index in images array
> -            self.selected_image = 0
> +            sel = 0
>               for i in range(len(self.cf.images)):
>                   if self.cf.images[i].title == title:
> -                    self.selected_image = i
> +                    sel = i
>                       break
>   
>           # If the selected (default) image doesn't exist we select the first entry
> -        if self.selected_image > len(self.cf.images):
> +        if sel > len(self.cf.images):
>               logging.warning("Default image not found")
> -            self.selected_image = 0
> +            sel = 0
> +
> +        return sel
> +
> +    def run(self):
> +        timeout = int(self.cf.timeout)
> +        self.selected_image = self.image_index()
> +
>           self.isdone = False
>           while not self.isdone:
>               self.run_main(timeout)
> @@ -626,7 +631,7 @@ def run_grub(file, entry, fs, cfg_args):
>       if interactive and not list_entries:
>           curses.wrapper(run_main)
>       else:
> -        sel = g.cf.default
> +        sel = g.image_index()
>   
>       # set the entry to boot as requested
>       if entry is not None:

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Ian Campbell]

On Mon, 2014-10-27 at 12:35 -0400, Boris Ostrovsky wrote:
> On 10/27/2014 12:00 PM, Simon Rowe wrote:
> > Changes to handle non-numeric default attributes for grub2 caused run_grub()
> > to attempt to index into the images list using a string. Pull out the code
> > that handles submenus into a new function and use that to ensure sel is
> > numeric.
> >
> > Reported-by: David Scott <dave.scott@citrix.com>
> > Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>
> 
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

And applied as a bug fix (I'm not sure if Konrad's response was intended
as a release-ack).

> 
> > ---
> >   tools/pygrub/src/pygrub |   23 ++++++++++++++---------
> >   1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> > index 1ae34a2..4cc846f 100644
> > --- a/tools/pygrub/src/pygrub
> > +++ b/tools/pygrub/src/pygrub
> > @@ -456,11 +456,9 @@ class Grub:
> >           del f
> >           self.cf.parse(buf)
> >   
> > -    def run(self):
> > -        timeout = int(self.cf.timeout)
> > -
> > +    def image_index(self):
> >           if self.cf.default.isdigit():
> > -            self.selected_image = int(self.cf.default)
> > +            sel = int(self.cf.default)
> >           else:
> >               # We don't fully support submenus. Look for the leaf value in
> >               # "submenu0>submenu1>...>menuentry" and hope that it's unique.
> > @@ -472,16 +470,23 @@ class Grub:
> >                       break
> >   
> >               # Map string to index in images array
> > -            self.selected_image = 0
> > +            sel = 0
> >               for i in range(len(self.cf.images)):
> >                   if self.cf.images[i].title == title:
> > -                    self.selected_image = i
> > +                    sel = i
> >                       break
> >   
> >           # If the selected (default) image doesn't exist we select the first entry
> > -        if self.selected_image > len(self.cf.images):
> > +        if sel > len(self.cf.images):
> >               logging.warning("Default image not found")
> > -            self.selected_image = 0
> > +            sel = 0
> > +
> > +        return sel
> > +
> > +    def run(self):
> > +        timeout = int(self.cf.timeout)
> > +        self.selected_image = self.image_index()
> > +
> >           self.isdone = False
> >           while not self.isdone:
> >               self.run_main(timeout)
> > @@ -626,7 +631,7 @@ def run_grub(file, entry, fs, cfg_args):
> >       if interactive and not list_entries:
> >           curses.wrapper(run_main)
> >       else:
> > -        sel = g.cf.default
> > +        sel = g.image_index()
> >   
> >       # set the entry to boot as requested
> >       if entry is not None:
> 

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Konrad Rzeszutek Wilk]

On Tue, Nov 04, 2014 at 10:46:37AM +0000, Ian Campbell wrote:
> On Mon, 2014-10-27 at 12:35 -0400, Boris Ostrovsky wrote:
> > On 10/27/2014 12:00 PM, Simon Rowe wrote:
> > > Changes to handle non-numeric default attributes for grub2 caused run_grub()
> > > to attempt to index into the images list using a string. Pull out the code
> > > that handles submenus into a new function and use that to ensure sel is
> > > numeric.
> > >
> > > Reported-by: David Scott <dave.scott@citrix.com>
> > > Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>
> > 
> > Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> And applied as a bug fix (I'm not sure if Konrad's response was intended
> as a release-ack).

Yes. Thank you for checking this patch in.
> 
> > 
> > > ---
> > >   tools/pygrub/src/pygrub |   23 ++++++++++++++---------
> > >   1 file changed, 14 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> > > index 1ae34a2..4cc846f 100644
> > > --- a/tools/pygrub/src/pygrub
> > > +++ b/tools/pygrub/src/pygrub
> > > @@ -456,11 +456,9 @@ class Grub:
> > >           del f
> > >           self.cf.parse(buf)
> > >   
> > > -    def run(self):
> > > -        timeout = int(self.cf.timeout)
> > > -
> > > +    def image_index(self):
> > >           if self.cf.default.isdigit():
> > > -            self.selected_image = int(self.cf.default)
> > > +            sel = int(self.cf.default)
> > >           else:
> > >               # We don't fully support submenus. Look for the leaf value in
> > >               # "submenu0>submenu1>...>menuentry" and hope that it's unique.
> > > @@ -472,16 +470,23 @@ class Grub:
> > >                       break
> > >   
> > >               # Map string to index in images array
> > > -            self.selected_image = 0
> > > +            sel = 0
> > >               for i in range(len(self.cf.images)):
> > >                   if self.cf.images[i].title == title:
> > > -                    self.selected_image = i
> > > +                    sel = i
> > >                       break
> > >   
> > >           # If the selected (default) image doesn't exist we select the first entry
> > > -        if self.selected_image > len(self.cf.images):
> > > +        if sel > len(self.cf.images):
> > >               logging.warning("Default image not found")
> > > -            self.selected_image = 0
> > > +            sel = 0
> > > +
> > > +        return sel
> > > +
> > > +    def run(self):
> > > +        timeout = int(self.cf.timeout)
> > > +        self.selected_image = self.image_index()
> > > +
> > >           self.isdone = False
> > >           while not self.isdone:
> > >               self.run_main(timeout)
> > > @@ -626,7 +631,7 @@ def run_grub(file, entry, fs, cfg_args):
> > >       if interactive and not list_entries:
> > >           curses.wrapper(run_main)
> > >       else:
> > > -        sel = g.cf.default
> > > +        sel = g.image_index()
> > >   
> > >       # set the entry to boot as requested
> > >       if entry is not None:
> > 
> 
> 

Re: [PATCH] pygrub: fix non-interactive parsing of grub1 config files

[Andrew Cooper]

On 27/10/14 16:00, Simon Rowe wrote:
> Changes to handle non-numeric default attributes for grub2 caused run_grub()
> to attempt to index into the images list using a string. Pull out the code
> that handles submenus into a new function and use that to ensure sel is
> numeric.
>
> Reported-by: David Scott <dave.scott@citrix.com>
> Signed-off-by: Simon Rowe <simon.rowe@eu.citrix.com>

Reviewed-and-tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/pygrub/src/pygrub |   23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index 1ae34a2..4cc846f 100644
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -456,11 +456,9 @@ class Grub:
>          del f
>          self.cf.parse(buf)
>  
> -    def run(self):
> -        timeout = int(self.cf.timeout)
> -
> +    def image_index(self):
>          if self.cf.default.isdigit():
> -            self.selected_image = int(self.cf.default)
> +            sel = int(self.cf.default)
>          else:
>              # We don't fully support submenus. Look for the leaf value in
>              # "submenu0>submenu1>...>menuentry" and hope that it's unique.
> @@ -472,16 +470,23 @@ class Grub:
>                      break
>  
>              # Map string to index in images array
> -            self.selected_image = 0
> +            sel = 0
>              for i in range(len(self.cf.images)):
>                  if self.cf.images[i].title == title:
> -                    self.selected_image = i
> +                    sel = i
>                      break
>  
>          # If the selected (default) image doesn't exist we select the first entry
> -        if self.selected_image > len(self.cf.images):
> +        if sel > len(self.cf.images):
>              logging.warning("Default image not found")
> -            self.selected_image = 0
> +            sel = 0
> +
> +        return sel
> +
> +    def run(self):
> +        timeout = int(self.cf.timeout)
> +        self.selected_image = self.image_index()
> +
>          self.isdone = False
>          while not self.isdone:
>              self.run_main(timeout)
> @@ -626,7 +631,7 @@ def run_grub(file, entry, fs, cfg_args):
>      if interactive and not list_entries:
>          curses.wrapper(run_main)
>      else:
> -        sel = g.cf.default
> +        sel = g.image_index()
>  
>      # set the entry to boot as requested
>      if entry is not None: