Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1614637 - Broken bash syntax in /etc/grub.d/01_fallback_counting [NEEDINFO]
Summary: Broken bash syntax in /etc/grub.d/01_fallback_counting
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: grub2
Version: 29
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Christian Glombek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1614756 1628663 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-08-10 06:20 UTC by Petr Pisar
Modified: 2020-02-20 19:29 UTC (History)
9 users (show)

Fixed In Version: grub2-2.02-62 grub2-2.02-62.fc29
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-10-23 15:07:47 UTC
Type: Bug
cglombek: needinfo? (pjones)


Attachments (Terms of Use)
Patch to clean this up. (5.04 KB, patch)
2018-10-04 18:26 UTC, Peter Jones
no flags Details | Diff

Description Petr Pisar 2018-08-10 06:20:10 UTC
# grub2-mkconfig >/boot/grub2/grub.cfg 
Generating grub configuration file ...
/etc/grub.d/01_fallback_counting: line 4: [: missing `]'
Found linux image: /boot/vmlinuz-4.18.0-0.rc8.git1.1.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.0-0.rc8.git1.1.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.0-0.rc8.git0.1.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.0-0.rc8.git0.1.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.0-0.rc7.git1.1.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.0-0.rc7.git1.1.fc29.x86_64.img
done

# rpm -qf /etc/grub.d/01_fallback_counting
grub2-tools-2.02-49.fc29.x86_64

# cat -n /etc/grub.d/01_fallback_counting
     1  #!/usr/bin/sh -e
     2  
     3  # Boot Counting
→    4  if [ "\${boot_counter}" -a "\${boot_success}" = "0"]; then
     5    if  [ "\${boot_counter}" = "0" -o "\${boot_counter}" = "-1" ]; then
     6      set default=1
     7      set boot_counter=-1
     8    else
     9      set boot_counter=$((\${boot_counter}-1))
    10    fi
    11    save_env boot_counter
    12  fi

You are missing a space before the closing bracket, thus you compare boot_success variable against '0]' string.

BTW, escaping the dollar sigils looks suspicious. I think it does not do what you want.

Comment 1 Jan Kurik 2018-08-14 08:37:41 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 29 development cycle.
Changing version to '29'.

Comment 2 Christian Glombek 2018-08-14 16:44:20 UTC
*** Bug 1614756 has been marked as a duplicate of this bug. ***

Comment 3 Villy Kruse 2018-08-14 18:04:13 UTC
(In reply to Petr Pisar from comment #0)
> # grub2-mkconfig >/boot/grub2/grub.cfg 
> Generating grub configuration file ...
> /etc/grub.d/01_fallback_counting: line 4: [: missing `]'
> Found linux image: /boot/vmlinuz-4.18.0-0.rc8.git1.1.fc29.x86_64
> Found initrd image: /boot/initramfs-4.18.0-0.rc8.git1.1.fc29.x86_64.img
> Found linux image: /boot/vmlinuz-4.18.0-0.rc8.git0.1.fc29.x86_64
> Found initrd image: /boot/initramfs-4.18.0-0.rc8.git0.1.fc29.x86_64.img
> Found linux image: /boot/vmlinuz-4.18.0-0.rc7.git1.1.fc29.x86_64
> Found initrd image: /boot/initramfs-4.18.0-0.rc7.git1.1.fc29.x86_64.img
> done
> 
> # rpm -qf /etc/grub.d/01_fallback_counting
> grub2-tools-2.02-49.fc29.x86_64
> 
> # cat -n /etc/grub.d/01_fallback_counting
>      1  #!/usr/bin/sh -e
>      2  
>      3  # Boot Counting
> →    4  if [ "\${boot_counter}" -a "\${boot_success}" = "0"]; then
>      5    if  [ "\${boot_counter}" = "0" -o "\${boot_counter}" = "-1" ]; then
>      6      set default=1
>      7      set boot_counter=-1
>      8    else
>      9      set boot_counter=$((\${boot_counter}-1))
>     10    fi
>     11    save_env boot_counter
>     12  fi
> 
> You are missing a space before the closing bracket, thus you compare
> boot_success variable against '0]' string.
> 
> BTW, escaping the dollar sigils looks suspicious. I think it does not do
> what you want.


I suppose this was supposed to create a grub.cfg snippet.  In that case it is missing a "cat << EOF" line and the corresponding "EOF" line.  As a shell script 
this does not make much sense.

Comment 4 Fedora Update System 2018-10-02 18:06:36 UTC
grub2-2.02-60.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 5 Villy Kruse 2018-10-02 18:59:03 UTC
(In reply to Fedora Update System from comment #4)
> grub2-2.02-60.fc29 has been submitted as an update to Fedora 29.
> https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Close; but no cigar:


[root@secondbux ~]# grub2-mkconfig -o /boot/grub2/grub.cfg
Generating grub configuration file ...
/etc/grub.d/01_fallback_counting: line 4: ${boot_counter}-1: syntax error: operand expected (error token is "${boot_counter}-1")
[root@secondbux ~]#

Comment 6 Christian Glombek 2018-10-02 19:27:05 UTC
would that be fixed by another parenthesis around \${boot_counter}?

Comment 7 Fedora Update System 2018-10-02 21:18:53 UTC
grub2-2.02-60.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 8 Villy Kruse 2018-10-02 21:38:29 UTC
(In reply to Christian Glombek from comment #6)
> would that be fixed by another parenthesis around \${boot_counter}?

The fix is:


diff --git a/util/grub.d/01_fallback_counting.in b/util/grub.d/01_fallback_counting.in
index afe06199a..31e898188 100644
--- a/util/grub.d/01_fallback_counting.in
+++ b/util/grub.d/01_fallback_counting.in
@@ -7,8 +7,8 @@ if [ "\${boot_counter}" -a "\${boot_success}" = "0" ]; then
     set default=1
     set boot_counter=-1
   else
-    set boot_counter=$((\${boot_counter}-1))
+    set boot_counter=\$((\${boot_counter}-1))
   fi
   save_env boot_counter
 fi
-EOF
\ No newline at end of file
+EOF

Comment 9 Adam Williamson 2018-10-03 16:03:24 UTC
This appears to break initial install. Some netinst tests failed in the Fedora-29-20181002.n.0 compose openQA testing, like this one:

https://openqa.fedoraproject.org/tests/287990

They failed with bootloader install error, program.log shows:

    01:42:30,298 INF program: Generating grub configuration file ...
    01:42:30,299 INF program: /etc/grub.d/01_fallback_counting: line 4: ${boot_counter}-1: syntax error: operand expected (error token is "${boot_counter}-1")
    01:42:30,300 DBG program: Return code: 1

Those tests seem to have installed this version of grub2 (as it was in updates-testing at the time they ran). In comparison, DVD install tests that deployed the previous build (-59) worked fine.

Comment 10 Adam Williamson 2018-10-03 19:51:36 UTC
oh, that's the same bug villy saw, now I look closer at the previous comments. So, consider this confirmation!

Comment 11 Fedora Update System 2018-10-03 20:21:42 UTC
grub2-2.02-61.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 12 Villy Kruse 2018-10-04 07:37:31 UTC
Still broken.  Just now it is a grub syntax issue:

Script started on 2018-10-04 09:20:46+02:00
[root@secondbux ~]# grub2-mkconfig -o /boot/grub2/grub.cfg
Generating grub configuration file ...
Found linux image: /boot/vmlinuz-4.18.11-301.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.11-301.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.11-300.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.11-300.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.10-300.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.10-300.fc29.x86_64.img
Found linux image: /boot/vmlinuz-0-rescue-9b6f90dd0fd94117aedaf2cc29d11cf5
Found initrd image: /boot/initramfs-0-rescue-9b6f90dd0fd94117aedaf2cc29d11cf5.img
error: ../grub-core/script/lexer.c:337:$.
error: ../grub-core/script/lexer.c:337:syntax error.
error: ../grub-core/script/lexer.c:337:Incorrect command.
error: ../grub-core/script/lexer.c:337:syntax error.
Syntax error at line 79
Syntax errors are detected in generated GRUB config file.
Ensure that there are no errors in /etc/default/grub
and /etc/grub.d/* files or please file a bug report with
/boot/grub2/grub.cfg.new file attached.
[root@secondbux ~]# exit
exit

Script done on 2018-10-04 09:21:27+02:00

Apparently grub does not like the "$((${boot_counter}-1))" construction.

Comment 13 Christian Glombek 2018-10-04 08:42:00 UTC
At this point I'm just guessing, but maybe \$((\${boot_counter})-1) will work?

Comment 14 Milos Vyletel 2018-10-04 10:35:53 UTC
Nope. that does not help either

    set boot_counter=\$((\${boot_counter})-1)

$ sudo grub2-mkconfig -o /etc/grub2-efi.cfg
Generating grub configuration file ...
Found linux image: /boot/vmlinuz-4.18.11-301.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.11-301.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.10-300.fc29.x86_64
Found initrd image: /boot/initramfs-4.18.10-300.fc29.x86_64.img
Found linux image: /boot/vmlinuz-4.18.10-200.fc28.x86_64
Found initrd image: /boot/initramfs-4.18.10-200.fc28.x86_64.img
Found linux image: /boot/vmlinuz-0-rescue-ee12a31aa29d41d6b9fd3d51df039fb2
Found initrd image: /boot/initramfs-0-rescue-ee12a31aa29d41d6b9fd3d51df039fb2.img
Adding boot menu entry for EFI firmware configuration
error: ../grub-core/script/lexer.c:337:$.
error: ../grub-core/script/lexer.c:337:syntax error.
error: ../grub-core/script/lexer.c:337:Incorrect command.
error: ../grub-core/script/lexer.c:337:syntax error.
Syntax error at line 84
Syntax errors are detected in generated GRUB config file.
Ensure that there are no errors in /etc/default/grub
and /etc/grub.d/* files or please file a bug report with
/etc/grub2-efi.cfg.new file attached.

Comment 15 Villy Kruse 2018-10-04 10:58:16 UTC
In the grub documentation, there is not mention of arithmetic expression, so it may not be possible to take the value of boot_counter and increment it from within grub itself.  I must admit I don't understand the real purpose of this piece of code, but I suspect that someone must reconsider the solution strategy, as this looks like a dead end.

Comment 16 Christian Glombek 2018-10-04 12:03:29 UTC
The purpose of this is to count down failing boot attempts.

If arithmetic ops aren't supported, maybe the right way to do this is to add a decrement function, analogous to https://github.com/rhboot/grub2/blob/fedora-30/util/grub-editenv.c#L270-L295 ?

Comment 17 Fedora Update System 2018-10-04 18:00:30 UTC
grub2-2.02-61.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 18 Peter Jones 2018-10-04 18:26:01 UTC
Created attachment 1490686 [details]
Patch to clean this up.

(In reply to Christian Glombek from comment #16)
> The purpose of this is to count down failing boot attempts.
> 
> If arithmetic ops aren't supported, maybe the right way to do this is to add
> a decrement function, analogous to
> https://github.com/rhboot/grub2/blob/fedora-30/util/grub-editenv.c#L270-L295
> ?

That's probably the right idea; there's also a logic problem there where we're testing if it's -1 inside a test that it's 0.  Patch attached that should fix both issues.

I also made it increment "default" instead of setting it to 1, so that on the case where you've installed 2 kernels in a row that don't work, but never tried them, it will still (eventually) converge on the last working kernel.

Comment 19 Peter Jones 2018-10-04 18:28:05 UTC
(note that the patch as posted /replaces/ the existing patches.)

Comment 20 Peter Jones 2018-10-04 20:28:41 UTC
I still don't understand how this is actually getting used, though - what increments boot_counter?  What resets it when we get a new kernel?

Comment 21 Adam Williamson 2018-10-04 20:48:45 UTC
pjones: there's some explanation on what he's trying to do here:

https://medium.com/@c.glombek/fedora-iot-gsoc-2018-final-report-c7a048dc8e80

It seems to be basically about automatically rolling back ostree upgrades on failed boot. Poking about in the links from that, the thing called 'greenboot' appears to be what's going to set the counter:

https://github.com/LorbusChris/greenboot/search?q=boot_counter&unscoped_q=boot_counter

The package for it recently passed review:

https://bugzilla.redhat.com/show_bug.cgi?id=1615895

Comment 22 Fedora Update System 2018-10-04 21:25:52 UTC
grub2-2.02-62.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 23 Peter Jones 2018-10-05 14:47:23 UTC
Alright, so that seems to mean grub2-mkconfig needs to test that we're actually using that, or else we're just going to record a failure every time.  Is "systemctl -q is-enabled greenboot.service" sufficient to tell that?

Comment 24 Adam Williamson 2018-10-05 16:18:50 UTC
Perhaps this script itself could be moved to that greenboot package, too?

Comment 25 Christian Glombek 2018-10-05 17:27:11 UTC
(In reply to Peter Jones from comment #18)
> That's probably the right idea; there's also a logic problem there where
> we're testing if it's -1 inside a test that it's 0.  Patch attached that
> should fix both issues.
> 
> I also made it increment "default" instead of setting it to 1, so that on
> the case where you've installed 2 kernels in a row that don't work, but
> never tried them, it will still (eventually) converge on the last working
> kernel.

The logic was sound, it was checking for boot_counter=-1 inside a test for boot_success=0, not boot_counter=0.

Always setting default=1 is in that case also desired, as this is intended for ostree-based systems to choose the rollback deployment which is adressed this way.

boot_counter, same as boot_success which was introduced with menu auto-hide, need to be re-set from userspace.
The (now removed) test [ "\${boot_counter}" -a "\${boot_success}" = "0"] is supposed to be enough of an indicator for grub to activate this behavior. I don't think a test for that in mkconfig is necessary for this.

This patch changes the logic in a way not intended; there was actually a need for the nested logic, please revert this.

Comment 26 Christian Glombek 2018-10-05 17:31:51 UTC
Just to make things clear: The boot_counter is a count down, and is re-set in userspace after installing an upgrade and before rebooting.

Comment 27 Fedora Update System 2018-10-05 18:22:46 UTC
grub2-2.02-62.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-d4d38d8885

Comment 28 Villy Kruse 2018-10-08 05:28:19 UTC
*** Bug 1628663 has been marked as a duplicate of this bug. ***

Comment 29 Fedora Update System 2018-10-09 00:05:32 UTC
grub2-2.02-62.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report.

Comment 30 Christian Glombek 2018-10-09 14:59:47 UTC
this is still not fixed, new PR in https://github.com/rhboot/grub2/pull/42


Note You need to log in before you can comment on or make changes to this bug.