Hi Michael,
not sure if you wanted to add more reviewers as the queue it is in right now is rather unmaintained.
Minimal review:
- changelog still points at UNRELEASED
- changelog has whitespace damage in line 5
- This is new right - why does it have "(offset 18 lines)"
- local (as used in at the head of is_multipath is undefined in posix and this uses only /bin/sh (would that be a problem?)
- quote the usage of variables to avoid word splitting if things go wrong
- but do we need to be concerned if "dmsetup" is available and if so should check ahead of time if the binary is there?
- maybe break the upper part of is_multipath into a fucntion like "get_uuid_from_dev" for readability and reusability
- the call in
GRUB_DEVICE=/dev/disk/by-id/dm-uuid-$(dmsetup info -c --noheadings -o uuid $GRUB_DEVICE)
essentially is the same as the first one in is_multipath
uuid=$(dmsetup info -c --noheadings -o uuid $dev 2>/dev/null)
Then you go through lengths to "potentially" strip a part-* and get the UUID for the probing.
But on GRUB_DEVICE= you don't do that.
So the result used for GRUB_DEVICE could still be partition symlink which according to your changelog is what you want to avoid right?
I'd expect that the second call to set GRUB_DEVICE would make use of that "get_uuid_from_dev" function I suggested above - otherwise it could end up as: /dev/disk/by-id/dm-uuid-part-... right?
On Fri, 2 Aug 2019, 18:04 Christian Ehrhardt , <
<email address hidden>> wrote:
> Review: Needs Fixing
>
> Hi Michael,
> not sure if you wanted to add more reviewers as the queue it is in right
> now is rather unmaintained.
>
I've pinged various foundations people out of band to look at this.
> Minimal review:
>
Thanks!
- changelog still points at UNRELEASED
>
I think I can be relied on to fix that before uploading ;)
- changelog has whitespace damage in line 5
>
Ack.
- This is new right - why does it have "(offset 18 lines)"
>
Hm, maybe I didn't update my branch before hacking.
- local (as used in at the head of is_multipath is undefined in posix and
> this uses only /bin/sh (would that be a problem?)
>
Well it's supported by dash and I doubt we'll ever move to an even more
militantly POSIX shell than that?
- quote the usage of variables to avoid word splitting if things go wrong
>
Ack.
- but do we need to be concerned if "dmsetup" is available and if so should
> check ahead of time if the binary is there?
>
This is covered by the general check for the first dmsetup invocation
failing, or at least that's the idea.
- maybe break the upper part of is_multipath into a fucntion like
> "get_uuid_from_dev" for readability and reusability
>
Yes, that would probably make sense
- the call in
> GRUB_DEVICE=/dev/disk/by-id/dm-uuid-$(dmsetup info -c --noheadings -o
> uuid $GRUB_DEVICE)
> essentially is the same as the first one in is_multipath
> uuid=$(dmsetup info -c --noheadings -o uuid $dev 2>/dev/null)
> Then you go through lengths to "potentially" strip a part-* and get the
> UUID for the probing.
> But on GRUB_DEVICE= you don't do that.
> So the result used for GRUB_DEVICE could still be partition symlink
> which according to your changelog is what you want to avoid right?
>
No ;) grub-probe --target device / returns the device mapper device for the
partition. But you need to check if the underlying device is multipath, not
the partition. You still want GRUB_DEVICE to be a link to the partition
though.
I'd expect that the second call to set GRUB_DEVICE would make use of that
> "get_uuid_from_dev" function I suggested above - otherwise it could end up
> as: /dev/disk/by-id/dm-uuid-part-... right?
>
We want it to end up as that... Or at least that was my reading of the bug.
New version, mostly the same but I've convinced myself that multipath devices always have a uuid starting with mpath- so that makes things a bit simpler. And I've changed to use a GRUB_DEVICE of /dev/mapper/$dm_name, which is what curtin has been overriding this to. Fixed up the other small things pointed out too.
I'm happy to apply the patch there for you though, or you can submit a new PR with the patch included, using git-dpm:
git clone ...
git-dpm checkout-patched
[...]
git am <your patch>
or
vi, git add, etc.
[...]
git commit --amend # add Patch-Name: at the end of headers so your patch is named correctly; we use ubuntu-* names for ubuntu patches, and separate works with -.
git-dpm update-patches
vi debian/changelog # add changelog entry
git add debian/changelog
git commit --amend # update the commit message with your changelog entry instead of "update patches"
d/patches/boot-from-multipath-dependent-symlink.patch: when booting from a multipath device, set root= to a UUID dependent symlink, not a partition UUID (which in general will be reachable by multiple paths!) (LP: #1429327)
New changelog entries:
[ Mathieu Trudel-Lapierre ]
* debian/patches/ubuntu-add-devicetree-command-support.patch: import patch
into git-dpm: drop [PATCH] tag and add Patch-Name.
[ Didier Roche ]
* debian/patches/ubuntu-zfs-enhance-support.patch
- Don't patch autoregenerated files.
- rewrite generate MenuMeta implementation in shell (LP: #1834095)
mawk doesn't support \s and other array features.
+ Change \s by their space or tab equivalent.
+ Rewrite the menumeta generation in pure shell, which is easier to
debug, keeping globally the same algorithm
+ Support i18n in entry name generation.
Co-authored with Jean-Baptiste.
- Resplit all patches in debian/patches/*, so that we have upstreamable
and non upstreamable parts separate. Also, any change in 10_linux patch
will be reflected in 10_linux_zfs.
- Always import pools (using force), as we don't mount them. Ensure also
that we don't update the host cache, as we import all pools, and not
only those attached to that system.
New changelog entries:
* Merge against Debian; remaining changes:
- debian/control: Update Vcs fields for code location on Ubuntu.
- debian/control: Breaks shim (<< 13).
- debian/patches/linuxefi.patch: Secure Boot support: use newer patchset
from rhboot repo, flattened to a single patch.
- debian/patches/install_signed.patch, grub-install-extra-removable.patch:
- Make sure if we install shim; it should also be exported as the default
bootloader to install later to a removable path, if we do.
- Rework grub-install-extra-removable.patch to reverse its logic: in the
default case, install the bootloader to /EFI/BOOT, unless we're trying
to install on a removable device, or explicitly telling grub *not* to
do it.
- Install a BOOT.CSV for fallback to use.
- Make sure postinst and templates know about the replacement of --force-extra-removable with --no-extra-removable.
- debian/patches/ubuntu-support-initrd-less-boot.patch: allow non-initrd
boot config.
- debian/patches/ubuntu-add-initrd-less-boot-fallback.patch: If a kernel
fails to boot without initrd, we will fallback to trying to boot the
kernel with an initrd.
- debian/patches/ubuntu-mkconfig-leave-breadcrumbs.patch: make sure
grub-mkconfig leaves a trace of what files were sourced to help generate
the config we're building.
- debian/patches/ubuntu-efi-console-set-text-mode-as-needed.patch: in EFI
console, only set text-mode when we're actually going to need it.
- debian/patches/ubuntu-zfs-enhance-support.patch: Better ZFS grub support.
- Disable os-prober for ppc64el on the PowerNV platform, to reduce the
number of entries/clutter from other OSes in Petitboot
- debian/patches/ubuntu-shorter-version-info.patch: Only show the upstream
version in menu and console, and hide the package one in a
package_version variable.
- Verify that the current and newer kernels are signed when grub is
updated, to make sure people do not accidentally shutdown without a
signed kernel.
- debian/default/grub: replace GRUB_HIDDEN_* variables with the less
confusing GRUB_TIMEOUT_STYLE=hidden.
- debian/rules: shuffle files around for now to keep build artefacts
for signing at the same location as they were expected by Launchpad.
- debian/rules, debian/control: enable dh-systemd.
- debian/grub-common.install.in: install the systemd unit that's part of
initrd fallback handling, missed when the feature landed.
- debian/build-efi-images: add http module to NET_MODULES.
* debian/patches/linuxefi*.patch: Flatten linuxefi patches into one.
* debian/patches: rename patches to use "-" as a separator rather than "_".
* debian/patches: rename Ubuntu-specific patches and commits to add "ubuntu"
so it's clearer which are new or changed when doing a merge.
* debian/patches/ubuntu-fix-lzma-decompressor-objcopy.patch: fix FTBFS due
to objcopy building an invalid binary padded with zeroes (LP: #1833234)
* debian/patches/ubuntu-clear-invalid-initrd-spacing.patch: clear up invalid
spacing for the initrd command when not using early initrds.
* debian/patches/ubuntu-add-initrd-less-boot-fallback.patch: move the initrd
boot success/failure service to start later at boot time. (LP: #1823391)
* debian/patches/fix-lockdown.patch: Drop lockdown patch from Debian, which
breaks with new linuxefi patchset.
* debian/patches/ubuntu-temp-keep-auto-nvram.patch: Temporarily keep the
--auto-nvram option we previously had as a supported option in grub-install
(with no effect now), to avoid breaking upgrades. "auto-nvram" is default
behavior now that we use libefivar instead of calling efibootmgr.
[ James Clarke ]
* Only Build-Depend on libefiboot-dev and libefivar-dev on Linux
architectures, since they're Linux-only.
New changelog entries:
[ Will Thompson ]
* Fix --disable-quiet-boot.
[ Steve Langasek ]
* If we don't have writable grubenv and we're on EFI, always show the menu
(merged from Ubuntu).
[ Steve McIntyre ]
* Make all the signed EFI arches have a Recommends: from
grub-efi-ARCH-signed to shim-signed, not just amd64.
Closes: #931038
* Add myself to Uploaders
[ Colin Watson ]
* Squash linuxefi* patches into a single patch.
Hi Michael,
not sure if you wanted to add more reviewers as the queue it is in right now is rather unmaintained.
Minimal review:
- changelog still points at UNRELEASED
- changelog has whitespace damage in line 5
- This is new right - why does it have "(offset 18 lines)"
- local (as used in at the head of is_multipath is undefined in posix and this uses only /bin/sh (would that be a problem?)
- quote the usage of variables to avoid word splitting if things go wrong
- but do we need to be concerned if "dmsetup" is available and if so should check ahead of time if the binary is there?
- maybe break the upper part of is_multipath into a fucntion like "get_uuid_from_dev" for readability and reusability
- the call in
GRUB_
essentially is the same as the first one in is_multipath
uuid=$(dmsetup info -c --noheadings -o uuid $dev 2>/dev/null)
Then you go through lengths to "potentially" strip a part-* and get the UUID for the probing.
But on GRUB_DEVICE= you don't do that.
So the result used for GRUB_DEVICE could still be partition symlink which according to your changelog is what you want to avoid right?
I'd expect that the second call to set GRUB_DEVICE would make use of that "get_uuid_from_dev" function I suggested above - otherwise it could end up as: /dev/disk/