[Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[YOUNG, MICHAEL A.]

This patch may help your issue with the default kernel setting on Fedora 
30 as it uses the setting of saved_entry or next_entry from the grubenv 
file to choose the default kernel which should override any setting picked 
up from if clauses in the grub.cfg file.

I have only done limited and somewhat imperfect testing on it and isn't a 
proper fix (which would use grubenv settings based on what is in the if 
clauses) but I think it should work in your case.

 	Michael Young

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[YOUNG, MICHAEL A.]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]

On Thu, 15 Aug 2019, Michael Young wrote:

> This patch may help your issue with the default kernel setting on Fedora 30 
> as it uses the setting of saved_entry or next_entry from the grubenv file to 
> choose the default kernel which should override any setting picked up from if 
> clauses in the grub.cfg file.
>
> I have only done limited and somewhat imperfect testing on it and isn't a 
> proper fix (which would use grubenv settings based on what is in the if 
> clauses) but I think it should work in your case.

The patch is actually attached this time.

 	Michael Young

[-- Attachment #2: 0001-read-grubenv-and-set-default-from-saved_entry-or-nex.patch --]
[-- Type: text/plain, Size: 3116 bytes --]

From 00dfa97d97f0c7c18d95ab47706660c7ab15b733 Mon Sep 17 00:00:00 2001
From: Michael Young <m.a.young@durham.ac.uk>
Date: Thu, 15 Aug 2019 00:16:44 +0100
Subject: [PATCH] read grubenv and set default from saved_entry or next_entry

This patch looks for a grubenv file in the same directory as the
grub.cfg file and includes it at front of the grub.cfg file when passed
to parse()

As the grubenv file consists of variable=value lines padded by hashes these
are treated as commands in parse() where it uses the value of saved_entry
or next_entry (if set) to set the default entry if a title matches.
---
 tools/pygrub/src/GrubConf.py |  9 +++++++++
 tools/pygrub/src/pygrub      | 13 ++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
index 594139bac7..c9a81fe936 100644
--- a/tools/pygrub/src/GrubConf.py
+++ b/tools/pygrub/src/GrubConf.py
@@ -383,6 +383,8 @@ class Grub2ConfigFile(_GrubConfigFile):
         img = None
         title = ""
         menu_level=0
+        img_count=0
+        default_title=""
         for l in lines:
             l = l.strip()
             # skip blank lines
@@ -408,6 +410,9 @@ class Grub2ConfigFile(_GrubConfigFile):
                     raise RuntimeError("syntax error: cannot nest menuentry (%d %s)" % (len(img),img))
                 img = []
                 title = title_match.group(1)
+                if title == default_title:
+                    setattr(self, 'default', img_count)
+                img_count += 1
                 continue
 
             if l.startswith("submenu"):
@@ -432,6 +437,10 @@ class Grub2ConfigFile(_GrubConfigFile):
 
             (com, arg) = grub_exact_split(l, 2)
         
+            if com == "saved_entry" or com == "next_entry":
+                default_title = arg
+                continue
+
             if com == "set":
                 (com,arg) = grub2_handle_set(arg)
                 
diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
index ce7ab0eb8c..267788795b 100755
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -454,8 +454,19 @@ class Grub:
         if self.__dict__.get('cf', None) is None:
             raise RuntimeError("couldn't find bootloader config file in the image provided.")
         f = fs.open_file(self.cf.filename)
+        fenv = self.cf.filename.replace("grub.cfg","grubenv")
+        if fenv != self.cf.filename and fs.file_exists(fenv):
+            # if grubenv file exists next to grub.cfg prepend it
+            fenvf = fs.open_file(fenv)
+            if sys.version_info[0] < 3:
+                fsep = "\n"
+            else:
+                fsep = b"\n"
+            buf = fsep.join((fenvf.read(FS_READ_MAX),f.read(FS_READ_MAX)))
+            del fenvf
         # limit read size to avoid pathological cases
-        buf = f.read(FS_READ_MAX)
+        else:
+            buf = f.read(FS_READ_MAX)
         del f
         if sys.version_info[0] < 3:
             self.cf.parse(buf)
-- 
2.21.0


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[Steven Haigh]

On 2019-08-15 09:56, YOUNG, MICHAEL A. wrote:
> On Thu, 15 Aug 2019, Michael Young wrote:
> 
>> This patch may help your issue with the default kernel setting on 
>> Fedora 30
>> as it uses the setting of saved_entry or next_entry from the grubenv 
>> file to
>> choose the default kernel which should override any setting picked up 
>> from if
>> clauses in the grub.cfg file.
>> 
>> I have only done limited and somewhat imperfect testing on it and 
>> isn't a
>> proper fix (which would use grubenv settings based on what is in the 
>> if
>> clauses) but I think it should work in your case.
> 
> The patch is actually attached this time.
> 
>  	Michael Young

Having a bit of a look here....

My test system grubenv file has:
# GRUB Environment Block
saved_entry=0
kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro 
rootflags=subvol=root selinux=0 rhgb quiet
boot_success=1
#################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################


The grub-set-default man page states:
SYNOPSIS
        grub-set-default [--boot-directory=DIR] MENU_ENTRY

        MENU_ENTRY
               A number, a menu item title or a menu item identifier.

Do I have to use a name for the title to match?

I notice I still have the second option selected with saved_entry=0

-- 
Steven Haigh

? netwiz@crc.id.au     ? http://www.crc.id.au
? +61 (3) 9001 6090    ? 0412 935 897

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[M A Young]

On Thu, 15 Aug 2019, Steven Haigh wrote:

> Having a bit of a look here....
> 
> My test system grubenv file has:
> # GRUB Environment Block
> saved_entry=0
> kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro
> rootflags=subvol=root selinux=0 rhgb quiet
> boot_success=1

It looks like the edit

                     raise RuntimeError("syntax error: cannot nest 
menuentry
(%d %s)" % (len(img),img))
                 img = []
                 title = title_match.group(1)
+                if title == default_title:
+                    setattr(self, 'default', img_count)
+                img_count += 1
                 continue
 
             if l.startswith("submenu"):

also needs a test for default_title == img_count though there may be 
variable types to consider in that comparison.

	Michael Young

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[YOUNG, MICHAEL A.]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Thu, 15 Aug 2019, Steven Haigh wrote:

> Having a bit of a look here....
>
> My test system grubenv file has:
> # GRUB Environment Block
> saved_entry=0
> kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro 
> rootflags=subvol=root selinux=0 rhgb quiet
> boot_success=1
> #################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################

I have attached a revision of the first patch which should handle a 
numeric saved_entry.

 	Michael Young

[-- Attachment #2: 0001-read-grubenv-and-set-default-from-saved_entry-or-nex.patch --]
[-- Type: text/plain, Size: 3527 bytes --]

From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001
From: Michael Young <m.a.young@durham.ac.uk>
Date: Thu, 15 Aug 2019 19:55:30 +0100
Subject: [PATCH] read grubenv and set default from saved_entry or next_entry

This patch looks for a grubenv file in the same directory as the
grub.cfg file and includes it at front of the grub.cfg file when passed
to parse()

As the grubenv file consists of variable=value lines padded by hashes these
are treated as commands in parse() where it uses the value of saved_entry
or next_entry (if set) to set the default entry if a title matches or is
a number.
---
 tools/pygrub/src/GrubConf.py | 11 +++++++++++
 tools/pygrub/src/pygrub      | 13 ++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
index 594139bac7..22e0948da2 100644
--- a/tools/pygrub/src/GrubConf.py
+++ b/tools/pygrub/src/GrubConf.py
@@ -383,6 +383,8 @@ class Grub2ConfigFile(_GrubConfigFile):
         img = None
         title = ""
         menu_level=0
+        img_count=0
+        default_title=""
         for l in lines:
             l = l.strip()
             # skip blank lines
@@ -408,6 +410,9 @@ class Grub2ConfigFile(_GrubConfigFile):
                     raise RuntimeError("syntax error: cannot nest menuentry (%d %s)" % (len(img),img))
                 img = []
                 title = title_match.group(1)
+                if title == default_title:
+                    setattr(self, 'default', img_count)
+                img_count += 1
                 continue
 
             if l.startswith("submenu"):
@@ -432,6 +437,10 @@ class Grub2ConfigFile(_GrubConfigFile):
 
             (com, arg) = grub_exact_split(l, 2)
         
+            if com == "saved_entry" or com == "next_entry":
+                default_title = arg
+                continue
+
             if com == "set":
                 (com,arg) = grub2_handle_set(arg)
                 
@@ -449,6 +458,8 @@ class Grub2ConfigFile(_GrubConfigFile):
             else:
                 logging.warning("Unknown directive %s" %(com,))
             
+        if default_title.isdigit():
+            setattr(self, 'default', default_title)
         if img is not None:
             raise RuntimeError("syntax error: end of file with open menuentry(%d %s)" % (len(img),img))
 
diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
index ce7ab0eb8c..267788795b 100755
--- a/tools/pygrub/src/pygrub
+++ b/tools/pygrub/src/pygrub
@@ -454,8 +454,19 @@ class Grub:
         if self.__dict__.get('cf', None) is None:
             raise RuntimeError("couldn't find bootloader config file in the image provided.")
         f = fs.open_file(self.cf.filename)
+        fenv = self.cf.filename.replace("grub.cfg","grubenv")
+        if fenv != self.cf.filename and fs.file_exists(fenv):
+            # if grubenv file exists next to grub.cfg prepend it
+            fenvf = fs.open_file(fenv)
+            if sys.version_info[0] < 3:
+                fsep = "\n"
+            else:
+                fsep = b"\n"
+            buf = fsep.join((fenvf.read(FS_READ_MAX),f.read(FS_READ_MAX)))
+            del fenvf
         # limit read size to avoid pathological cases
-        buf = f.read(FS_READ_MAX)
+        else:
+            buf = f.read(FS_READ_MAX)
         del f
         if sys.version_info[0] < 3:
             self.cf.parse(buf)
-- 
2.21.0


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[Steven Haigh]

On 2019-08-16 05:05, YOUNG, MICHAEL A. wrote:
> On Thu, 15 Aug 2019, Steven Haigh wrote:
> 
>> Having a bit of a look here....
>> 
>> My test system grubenv file has:
>> # GRUB Environment Block
>> saved_entry=0
>> kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro
>> rootflags=subvol=root selinux=0 rhgb quiet
>> boot_success=1
>> #################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################
> 
> I have attached a revision of the first patch which should handle a
> numeric saved_entry.

Hi Michael,

I tried this - and it successfully works for systems that have 
saved_entry=0.

I noticed that stock installs still have problems with updating grubenv 
from new kernel installs. I had to manually regenerate grub.cfg after 
upgrading to kernel 5.2.8. grubenv doesn't seem to get changed at all 
unless you manually use 'grub2-set-default 0'

$ rpm -qa | grep kernel | sort
kernel-5.1.15-300.fc30.x86_64
kernel-5.2.8-200.fc30.x86_64
kernel-core-5.1.15-300.fc30.x86_64
kernel-core-5.2.8-200.fc30.x86_64
kernel-headers-5.2.8-200.fc30.x86_64
kernel-modules-5.1.15-300.fc30.x86_64
kernel-modules-5.2.8-200.fc30.x86_64

$ rpm -qa | grep grub | sort
grub2-common-2.02-81.fc30.noarch
grub2-pc-2.02-81.fc30.x86_64
grub2-pc-modules-2.02-81.fc30.noarch
grub2-tools-2.02-81.fc30.x86_64
grub2-tools-efi-2.02-81.fc30.x86_64
grub2-tools-extra-2.02-81.fc30.x86_64
grub2-tools-minimal-2.02-81.fc30.x86_64
grubby-8.40-31.fc30.x86_64
grubby-deprecated-8.40-31.fc30.x86_64

$ cat /etc/default/grub
GRUB_TIMEOUT=1
GRUB_DEFAULT=saved
GRUB_DISABLE_SUBMENU=true
GRUB_TERMINAL_OUTPUT="console"
GRUB_CMDLINE_LINUX="audit=0 selinux=0 console=hvc0"
GRUB_DISABLE_RECOVERY="true"
GRUB_ENABLE_BLSCFG=false

It seems we still have issues with this configuration - but this is a 
Fedora 30 problem -  not Xen.

-- 
Steven Haigh

? netwiz@crc.id.au     ? http://www.crc.id.au
? +61 (3) 9001 6090    ? 0412 935 897

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[Steven Haigh]

On 2019-08-16 15:25, Steven Haigh wrote:
> On 2019-08-16 05:05, YOUNG, MICHAEL A. wrote:
>> On Thu, 15 Aug 2019, Steven Haigh wrote:
>> 
>>> Having a bit of a look here....
>>> 
>>> My test system grubenv file has:
>>> # GRUB Environment Block
>>> saved_entry=0
>>> kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro
>>> rootflags=subvol=root selinux=0 rhgb quiet
>>> boot_success=1
>>> #################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################
>> 
>> I have attached a revision of the first patch which should handle a
>> numeric saved_entry.
> 
> Hi Michael,
> 
> I tried this - and it successfully works for systems that have 
> saved_entry=0.
> 
> I noticed that stock installs still have problems with updating
> grubenv from new kernel installs. I had to manually regenerate
> grub.cfg after upgrading to kernel 5.2.8. grubenv doesn't seem to get
> changed at all unless you manually use 'grub2-set-default 0'
> 
> $ rpm -qa | grep kernel | sort
> kernel-5.1.15-300.fc30.x86_64
> kernel-5.2.8-200.fc30.x86_64
> kernel-core-5.1.15-300.fc30.x86_64
> kernel-core-5.2.8-200.fc30.x86_64
> kernel-headers-5.2.8-200.fc30.x86_64
> kernel-modules-5.1.15-300.fc30.x86_64
> kernel-modules-5.2.8-200.fc30.x86_64
> 
> $ rpm -qa | grep grub | sort
> grub2-common-2.02-81.fc30.noarch
> grub2-pc-2.02-81.fc30.x86_64
> grub2-pc-modules-2.02-81.fc30.noarch
> grub2-tools-2.02-81.fc30.x86_64
> grub2-tools-efi-2.02-81.fc30.x86_64
> grub2-tools-extra-2.02-81.fc30.x86_64
> grub2-tools-minimal-2.02-81.fc30.x86_64
> grubby-8.40-31.fc30.x86_64
> grubby-deprecated-8.40-31.fc30.x86_64
> 
> $ cat /etc/default/grub
> GRUB_TIMEOUT=1
> GRUB_DEFAULT=saved
> GRUB_DISABLE_SUBMENU=true
> GRUB_TERMINAL_OUTPUT="console"
> GRUB_CMDLINE_LINUX="audit=0 selinux=0 console=hvc0"
> GRUB_DISABLE_RECOVERY="true"
> GRUB_ENABLE_BLSCFG=false
> 
> It seems we still have issues with this configuration - but this is a
> Fedora 30 problem -  not Xen.

Sorry, forgot to add this for using the functionality of saved_entry=0.

Tested-by: Steven Haigh <netwiz@crc.id.au>

Have not tested using a string as the entry - as all of my installs seem 
to have other problems wrt semi-related fedora issues.

-- 
Steven Haigh

? netwiz@crc.id.au     ? http://www.crc.id.au
? +61 (3) 9001 6090    ? 0412 935 897

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry

[Steven Haigh]

Just wanted to give this a quick followup... Did this end up 
progressing?

On Fri, Aug 16, 2019 at 3:37 PM, Steven Haigh <netwiz@crc.id.au> wrote:
> On 2019-08-16 15:25, Steven Haigh wrote:
>> On 2019-08-16 05:05, YOUNG, MICHAEL A. wrote:
>>> On Thu, 15 Aug 2019, Steven Haigh wrote:
>>> 
>>>> Having a bit of a look here....
>>>> 
>>>> My test system grubenv file has:
>>>> # GRUB Environment Block
>>>> saved_entry=0
>>>> kernelopts=root=UUID=5346b4d9-885f-4673-8aff-04a16bf1971a ro
>>>> rootflags=subvol=root selinux=0 rhgb quiet
>>>> boot_success=1
>>>> #################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################################
>>> 
>>> I have attached a revision of the first patch which should handle a
>>> numeric saved_entry.
>> 
>> Hi Michael,
>> 
>> I tried this - and it successfully works for systems that have 
>> \x7fsaved_entry=0.
>> 
>> I noticed that stock installs still have problems with updating
>> grubenv from new kernel installs. I had to manually regenerate
>> grub.cfg after upgrading to kernel 5.2.8. grubenv doesn't seem to get
>> changed at all unless you manually use 'grub2-set-default 0'
>> 
>> $ rpm -qa | grep kernel | sort
>> kernel-5.1.15-300.fc30.x86_64
>> kernel-5.2.8-200.fc30.x86_64
>> kernel-core-5.1.15-300.fc30.x86_64
>> kernel-core-5.2.8-200.fc30.x86_64
>> kernel-headers-5.2.8-200.fc30.x86_64
>> kernel-modules-5.1.15-300.fc30.x86_64
>> kernel-modules-5.2.8-200.fc30.x86_64
>> 
>> $ rpm -qa | grep grub | sort
>> grub2-common-2.02-81.fc30.noarch
>> grub2-pc-2.02-81.fc30.x86_64
>> grub2-pc-modules-2.02-81.fc30.noarch
>> grub2-tools-2.02-81.fc30.x86_64
>> grub2-tools-efi-2.02-81.fc30.x86_64
>> grub2-tools-extra-2.02-81.fc30.x86_64
>> grub2-tools-minimal-2.02-81.fc30.x86_64
>> grubby-8.40-31.fc30.x86_64
>> grubby-deprecated-8.40-31.fc30.x86_64
>> 
>> $ cat /etc/default/grub
>> GRUB_TIMEOUT=1
>> GRUB_DEFAULT=saved
>> GRUB_DISABLE_SUBMENU=true
>> GRUB_TERMINAL_OUTPUT="console"
>> GRUB_CMDLINE_LINUX="audit=0 selinux=0 console=hvc0"
>> GRUB_DISABLE_RECOVERY="true"
>> GRUB_ENABLE_BLSCFG=false
>> 
>> It seems we still have issues with this configuration - but this is a
>> Fedora 30 problem -  not Xen.
> 
> Sorry, forgot to add this for using the functionality of 
> saved_entry=0.
> 
> Tested-by: Steven Haigh <netwiz@crc.id.au>
> 
> Have not tested using a string as the entry - as all of my installs 
> seem to have other problems wrt semi-related fedora issues.
> 
> --
> Steven Haigh
> 
> ? netwiz@crc.id.au     ? http://www.crc.id.au
> ? +61 (3) 9001 6090    ? 0412 935 897



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[Ian Jackson]

Steven Haigh writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry"):
> Just wanted to give this a quick followup... Did this end up 
> progressing?

Hi.  I'm a tools maintainer and probably your best bet for a review
etc of this patch.  If, next time, you use add_maintainers.pl or
something, you'll end up CCing the maintainer and your mail won't get
dropped.  Anyway, thanks for chasing it up through a back channel :-).

Michael Young <m.a.young@durham.ac.uk>:
> From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001
> From: Michael Young <m.a.young@durham.ac.uk>
> Date: Thu, 15 Aug 2019 19:55:30 +0100
> Subject: [PATCH] read grubenv and set default from saved_entry or next_entry
> 
> This patch looks for a grubenv file in the same directory as the
> grub.cfg file and includes it at front of the grub.cfg file when passed
> to parse()

Thanks for the contribution.  I reviewed the patch and I have some
comments.

I think this patch would be less confusing if it were two patches.
One which does the saved/next entry, and one which reads grubenv.  Do
you think that would make sense ?  If so I would appreciate it if you
would split it up (and write a nice explanatory commit message about
the saved_entry stuff).

> As the grubenv file consists of variable=value lines padded by hashes these
> are treated as commands in parse() where it uses the value of saved_entry
> or next_entry (if set) to set the default entry if a title matches or is
> a number.

I like reusing the parser.

> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index ce7ab0eb8c..267788795b 100755
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -454,8 +454,19 @@ class Grub:
>          if self.__dict__.get('cf', None) is None:
>              raise RuntimeError("couldn't find bootloader config file in the image provided.")
>          f = fs.open_file(self.cf.filename)
> +        fenv = self.cf.filename.replace("grub.cfg","grubenv")

I find this filename hackery rather unprincipled.  I'm not entirely
sure I can see a better way, given the way cfg_list is constructed.
Can you think of a less hacky approach ?

What would happen in future if we provided a way to control the
grub.cfg read by pygrub and a user configured it to (say)
`grub.cfg.old' ?  Would we really want it to read `grubenv.old' ?

> +        if fenv != self.cf.filename and fs.file_exists(fenv):
> +            # if grubenv file exists next to grub.cfg prepend it
> +            fenvf = fs.open_file(fenv)
> +            if sys.version_info[0] < 3:
> +                fsep = "\n"
> +            else:
> +                fsep = b"\n"
> +            buf = fsep.join((fenvf.read(FS_READ_MAX),f.read(FS_READ_MAX)))
> +            del fenvf
>          # limit read size to avoid pathological cases
> -        buf = f.read(FS_READ_MAX)
> +        else:
> +            buf = f.read(FS_READ_MAX)
>          del f
>          if sys.version_info[0] < 3:
>              self.cf.parse(buf)

Can we instead make the parser take a list ?  This business of
constructing a concatenated string is not very nice.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[YOUNG, MICHAEL A.]

On Wed, 11 Sep 2019, Ian Jackson wrote:

> Steven Haigh writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry"):
>> Just wanted to give this a quick followup... Did this end up
>> progressing?
>
> Hi.  I'm a tools maintainer and probably your best bet for a review
> etc of this patch.  If, next time, you use add_maintainers.pl or
> something, you'll end up CCing the maintainer and your mail won't get
> dropped.  Anyway, thanks for chasing it up through a back channel :-).
>
> Michael Young <m.a.young@durham.ac.uk>:
>> From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001
>> From: Michael Young <m.a.young@durham.ac.uk>
>> Date: Thu, 15 Aug 2019 19:55:30 +0100
>> Subject: [PATCH] read grubenv and set default from saved_entry or next_entry
>>
>> This patch looks for a grubenv file in the same directory as the
>> grub.cfg file and includes it at front of the grub.cfg file when passed
>> to parse()
>
> Thanks for the contribution.  I reviewed the patch and I have some
> comments.
>
> I think this patch would be less confusing if it were two patches.
> One which does the saved/next entry, and one which reads grubenv.  Do
> you think that would make sense ?  If so I would appreciate it if you
> would split it up (and write a nice explanatory commit message about
> the saved_entry stuff).

Yes, it makes sense to split it up, perhaps with a third patch for a 
format change of what is passed from pygrub to the backemds.

>> As the grubenv file consists of variable=value lines padded by hashes these
>> are treated as commands in parse() where it uses the value of saved_entry
>> or next_entry (if set) to set the default entry if a title matches or is
>> a number.
>
> I like reusing the parser.
>
>> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
>> index ce7ab0eb8c..267788795b 100755
>> --- a/tools/pygrub/src/pygrub
>> +++ b/tools/pygrub/src/pygrub
>> @@ -454,8 +454,19 @@ class Grub:
>>          if self.__dict__.get('cf', None) is None:
>>              raise RuntimeError("couldn't find bootloader config file in the image provided.")
>>          f = fs.open_file(self.cf.filename)
>> +        fenv = self.cf.filename.replace("grub.cfg","grubenv")
>
> I find this filename hackery rather unprincipled.  I'm not entirely
> sure I can see a better way, given the way cfg_list is constructed.
> Can you think of a less hacky approach ?
>
> What would happen in future if we provided a way to control the
> grub.cfg read by pygrub and a user configured it to (say)
> `grub.cfg.old' ?  Would we really want it to read `grubenv.old' ?
>

One option would be to do a fresh search for grubenv in the same places
we looked for grub.cfg.
Alternatively, it should be possible to do a more precise edit using
f.rpartition("/").

>> +        if fenv != self.cf.filename and fs.file_exists(fenv):
>> +            # if grubenv file exists next to grub.cfg prepend it
>> +            fenvf = fs.open_file(fenv)
>> +            if sys.version_info[0] < 3:
>> +                fsep = "\n"
>> +            else:
>> +                fsep = b"\n"
>> +            buf = fsep.join((fenvf.read(FS_READ_MAX),f.read(FS_READ_MAX)))
>> +            del fenvf
>>          # limit read size to avoid pathological cases
>> -        buf = f.read(FS_READ_MAX)
>> +        else:
>> +            buf = f.read(FS_READ_MAX)
>>          del f
>>          if sys.version_info[0] < 3:
>>              self.cf.parse(buf)
>
> Can we instead make the parser take a list ?  This business of
> constructing a concatenated string is not very nice.

Yes a list is probably the way to go, particularly as we might want to 
pass more file contents later for BLS support.

 	Michael Young

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[Ian Jackson]

Hi.  Thanks for the message.

Just one thing I wanted to reply to in your mail:

YOUNG, MICHAEL A. writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]"):
> On Wed, 11 Sep 2019, Ian Jackson wrote:
> > I find this filename hackery rather unprincipled.  I'm not entirely
> > sure I can see a better way, given the way cfg_list is constructed.
> > Can you think of a less hacky approach ?
...
> One option would be to do a fresh search for grubenv in the same places
> we looked for grub.cfg.
> 
> Alternatively, it should be possible to do a more precise edit using
> f.rpartition("/").

I don't feel I fully understand the implications, but either of these
seems like they might be good strategies to me.  I guess the former
risks finding an unrelated leftover grubenv somewhere which is
probably not desirable.

Anyway, I will leave this to your judgement.

Thanks for the rest of your comments.  I'll look forward to a revised
set of patches - thanks.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[Lars Kurth]

> On 7 Oct 2019, at 11:01, Ian Jackson <Ian.Jackson@citrix.com> wrote:
> 
> Hi.  Thanks for the message.
> 
> Just one thing I wanted to reply to in your mail:
> 
> YOUNG, MICHAEL A. writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]"):
>> On Wed, 11 Sep 2019, Ian Jackson wrote:
>>> I find this filename hackery rather unprincipled.  I'm not entirely
>>> sure I can see a better way, given the way cfg_list is constructed.
>>> Can you think of a less hacky approach ?
> ...
>> One option would be to do a fresh search for grubenv in the same places
>> we looked for grub.cfg.
>> 
>> Alternatively, it should be possible to do a more precise edit using
>> f.rpartition("/").
> 
> I don't feel I fully understand the implications, but either of these
> seems like they might be good strategies to me.  I guess the former
> risks finding an unrelated leftover grubenv somewhere which is
> probably not desirable.
> 
> Anyway, I will leave this to your judgement.
> 
> Thanks for the rest of your comments.  I'll look forward to a revised
> set of patches - thanks.

Hi all,

I am assuming there is no chance that this will make 4.13 with the freeze having passed.

But in any case, I wasn't sure whether Michael strictly will need it as the change can presumably be carried in a fedora patch q for Xen packages until it ends up upstream

Lars



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[Ian Jackson]

Lars Kurth writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]"):
> I am assuming there is no chance that this will make 4.13 with the freeze having passed.

Assuming we can get good quality patches, I would probably support a
freeze exception.  Failing that, this would be a strong candidate for
a backport.

> But in any case, I wasn't sure whether Michael strictly will need it
> as the change can presumably be carried in a fedora patch q for Xen
> packages until it ends up upstream

That's not really enough, because pygrub runs in the host but
interprets the guest filesystem.  Since we want to be able to boot
Fedora guests on non-Fedora hosts, that means that every host must
have this fix.

This is an inherent consequence of pygrub's design approach, and means
that I aggressively backport pygrub changes to older releases, so that
older hosts can boot newer guests.

But in the meantime putting this patch in the Fedora host packages is
not a terrible idea.  (There is a bit of a risk that if we deploy in
Fedora one algorith, and then upstream a different algorithm, we may
end up stuck with divergence, but I doubt this will be a big problem
here.)

thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel