Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 25/11/14 14:14, Ian Campbell wrote: On Fri, 2014-11-21 at 13:32 +, Andrew Cooper wrote: On 20/11/14 16:45, Boris Ostrovsky wrote: On 11/20/2014 11:15 AM, Ian Campbell wrote: On Thu, 2014-11-20 at 16:08 +, Andrew Cooper wrote: On 20/11/14 16:00, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? It would be BaseString not str, but I am fairly sure the classes have altered through Py2's history. I would not be any more confident with that as a solution as trying to correctly to start with. Actually isinstance(s, basestring) is what the webpage I was looking at said, but I cut-n-pasted the wrong thing. But regardless could it not do something like: if !isinstance(foo.cf.default, int): I think it would be the other way around, e.g. (not tested): diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub index aa7e562..7250f45 100644 --- a/tools/pygrub/src/pygrub +++ b/tools/pygrub/src/pygrub @@ -457,7 +457,9 @@ class Grub: self.cf.parse(buf) def image_index(self): -if self.cf.default.isdigit(): +if isinstance(self.cf.default, int) +sel = self.cf.default +elif if self.cf.default.isdigit(): sel = int(self.cf.default) else: # We don't fully support submenus. Look for the leaf value in but yes, this looks less intrusive (assuming this the only place where we'd hit this error). That does look plausibly like it would fix the issue. Is someone going to resubmit a patch along these lines then? Ian Unless you are NAKing my original patch, no. That is untested, and IMO only a gross hack around the complete type brokenness introduced by d1b93ea. I still firmly believe that my patch is the proper solution, which brings all the types back in line. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 20/11/14 16:45, Boris Ostrovsky wrote: On 11/20/2014 11:15 AM, Ian Campbell wrote: On Thu, 2014-11-20 at 16:08 +, Andrew Cooper wrote: On 20/11/14 16:00, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? It would be BaseString not str, but I am fairly sure the classes have altered through Py2's history. I would not be any more confident with that as a solution as trying to correctly to start with. Actually isinstance(s, basestring) is what the webpage I was looking at said, but I cut-n-pasted the wrong thing. But regardless could it not do something like: if !isinstance(foo.cf.default, int): I think it would be the other way around, e.g. (not tested): diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub index aa7e562..7250f45 100644 --- a/tools/pygrub/src/pygrub +++ b/tools/pygrub/src/pygrub @@ -457,7 +457,9 @@ class Grub: self.cf.parse(buf) def image_index(self): -if self.cf.default.isdigit(): +if isinstance(self.cf.default, int) +sel = self.cf.default +elif if self.cf.default.isdigit(): sel = int(self.cf.default) else: # We don't fully support submenus. Look for the leaf value in but yes, this looks less intrusive (assuming this the only place where we'd hit this error). That does look plausibly like it would fix the issue. However, I can't help but feeing that this is hacking around a broken patch in the first place. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On Fri, Nov 21, 2014 at 01:32:13PM +, Andrew Cooper wrote: On 20/11/14 16:45, Boris Ostrovsky wrote: On 11/20/2014 11:15 AM, Ian Campbell wrote: On Thu, 2014-11-20 at 16:08 +, Andrew Cooper wrote: On 20/11/14 16:00, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? It would be BaseString not str, but I am fairly sure the classes have altered through Py2's history. I would not be any more confident with that as a solution as trying to correctly to start with. Actually isinstance(s, basestring) is what the webpage I was looking at said, but I cut-n-pasted the wrong thing. But regardless could it not do something like: if !isinstance(foo.cf.default, int): I think it would be the other way around, e.g. (not tested): diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub index aa7e562..7250f45 100644 --- a/tools/pygrub/src/pygrub +++ b/tools/pygrub/src/pygrub @@ -457,7 +457,9 @@ class Grub: self.cf.parse(buf) def image_index(self): -if self.cf.default.isdigit(): +if isinstance(self.cf.default, int) +sel = self.cf.default +elif if self.cf.default.isdigit(): sel = int(self.cf.default) else: # We don't fully support submenus. Look for the leaf value in but yes, this looks less intrusive (assuming this the only place where we'd hit this error). That does look plausibly like it would fix the issue. However, I can't help but feeing that this is hacking around a broken patch in the first place. I cannot think of a reason you would ever feel that way! surreptitiously checks the roll of Xen 4.5 band-aid We know that the existing patches work fine for a host of Fedora families (15-21) (thought I need to double check that the testing framework that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s. The ones that I am worried about are the ExtLinux and such which I didn't realize would use 'pygrub'. Thought I just remembered a bug with OL7 grub entries - that is if you go in the interactive menu things broke down. Boris, do you remember if that was fixed or just 'deferred'? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 11/21/2014 12:09 PM, Konrad Rzeszutek Wilk wrote: On Fri, Nov 21, 2014 at 01:32:13PM +, Andrew Cooper wrote: That does look plausibly like it would fix the issue. However, I can't help but feeing that this is hacking around a broken patch in the first place. I cannot think of a reason you would ever feel that way! surreptitiously checks the roll of Xen 4.5 band-aid We know that the existing patches work fine for a host of Fedora families (15-21) (thought I need to double check that the testing framework that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s. The ones that I am worried about are the ExtLinux and such which I didn't realize would use 'pygrub'. Thought I just remembered a bug with OL7 grub entries - that is if you go in the interactive menu things broke down. Boris, do you remember if that was fixed or just 'deferred'? The only bug that I remember that had to do pygrub and OL7 was the fact that pygrub cannot parse grubenv (and therefore it's not really OL7-specific). We decided not to fix it. -boris ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? Also, in run_grub sel can be set to g.cf.default and then: if sel == -1: print No kernel image selected! sys.exit(1) I can't see where the -1 comes from though, and you aren't changing any -1 into -1, so maybe something more subtle is going on there. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 20/11/14 16:00, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? It would be BaseString not str, but I am fairly sure the classes have altered through Py2's history. I would not be any more confident with that as a solution as trying to correctly to start with. By far the least risky option at this point would be to revert the two identified commits in the comments of the original patch. Also, in run_grub sel can be set to g.cf.default and then: if sel == -1: print No kernel image selected! sys.exit(1) I can't see where the -1 comes from though, and you aren't changing any -1 into -1, so maybe something more subtle is going on there. Ian. sel comes either from g.image_index() which strictly is an integer, or pulled out of the loop immediately preceding and strictly an integer. I can't however find anything which could cause it to have the value -1. All error paths I can spot use 0 instead and load the first entry. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 11/20/2014 11:15 AM, Ian Campbell wrote: On Thu, 2014-11-20 at 16:08 +, Andrew Cooper wrote: On 20/11/14 16:00, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. c/s d1b93ea itself changed an an interface which previously used exclusively integers, to using strings in the case of a grub configuration with explicit default set, along with changing the code calling the interface to require a string. The default value for default remained as an integer. As a result, any Extlinux or Lilo configuration (which drives this interface exclusively with integers), or Grub configuration which doesn't explicitly declare a default will die with an AttributeError when attempting to call self.cf.default.isdigit() where default is an integer. Sadly, this AttributeError gets swallowed by the blanket ignore in the loop which searches partitions for valid bootloader configurations, causing the issue to be reported as Unable to find partition containing kernel This patch attempts to fix the issue by altering all parts of this interface to use strings, as opposed to integers. Would it be less invasive at this point in the release to have the place where this stuff is used do isinstance(s, str) and isinstance(s, int)? It would be BaseString not str, but I am fairly sure the classes have altered through Py2's history. I would not be any more confident with that as a solution as trying to correctly to start with. Actually isinstance(s, basestring) is what the webpage I was looking at said, but I cut-n-pasted the wrong thing. But regardless could it not do something like: if !isinstance(foo.cf.default, int): I think it would be the other way around, e.g. (not tested): diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub index aa7e562..7250f45 100644 --- a/tools/pygrub/src/pygrub +++ b/tools/pygrub/src/pygrub @@ -457,7 +457,9 @@ class Grub: self.cf.parse(buf) def image_index(self): -if self.cf.default.isdigit(): +if isinstance(self.cf.default, int) +sel = self.cf.default +elif if self.cf.default.isdigit(): sel = int(self.cf.default) else: # We don't fully support submenus. Look for the leaf value in but yes, this looks less intrusive (assuming this the only place where we'd hit this error). -boris blah = int(foo.cf.default) elif foo.cf.default.isdigit(): blah = whatever and avoid the confusion about what is/isn't a string class while still fixing the issue? sel comes either from g.image_index() which strictly is an integer, or pulled out of the loop immediately preceding and strictly an integer. Ah, good. I can't however find anything which could cause it to have the value -1. All error paths I can spot use 0 instead and load the first entry. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2
On 11/17/2014 11:59 AM, Andrew Cooper wrote: On 17/11/14 16:58, Ian Campbell wrote: On Mon, 2014-11-17 at 15:19 +, Andrew Cooper wrote: c/s d1b93ea causes substantial functional regressions in pygrub's ability to parse bootloader configuration files. Please can you and Boris both provide examples of (ideally real-world) configuration files which exhibit these failures as patches against tools/pygrub/examples/. Boris, in your case I mean the one which caused you to write the original patch, which I should have remembered to ask for at the time. I wanted to be able to parse grub2's default values set as strings, such as set default=Fedora, with Xen xen and Linux 3.17.0-rc3 or, for submenus set default=Advanced options for Fedora (with Xen hypervisor)Fedora, with Xen xen and Linux 3.17.0-rc3 Original pygrub would not be able to understand the string and default to zero. -boris Andy, in your case it would make sense to include at least one in this patch I think. Ian. examples/ doesn't help. The parsers themselves don't exhibit the bug. It is only when pygrub itself attempts to interact with the parsed config does the issue exhibits itself. I would have provided an example if it would have helped. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
