Hi Simon, Aaron,

thank you both for the review! It's somewhat reassuring to see my
initial reaction to `|| true` mirrored in others hehehe.

My initial patch did follow the precedent in os-probes/, mostly out of a
concern of changing the existing codebase too much. I've tested it
extensively and found no problems with it, but maybe we can/should take
a different approach here.

I've pushed a v2 that actually guards against `grub-probe` breaking the
script, by writing "bad" to `$type`. This should keep $type always
valid, hopefully even for future changes to grub-probe. I've kept the
rest of the code as close as possible to the original, mainly the part
of manually overriding the type to fuseblk, as:

1) I'm not 100% certain other scripts or packages rely on $type=fuseblk further 
on, and didn't want to break those by removing it or overriding it with "bad"
2) we don't want to preemptively unmount the partition and exit if grub-probe 
fails

Specifically for 2), addressing Aaron's comment as well: there are
partition types that are mounted by grub-mount but fail to be recognized
by grub-probe (that's the original cause for this LP bug). Certain types
of LVM2 members seem to be affected by this, and we *do* want the script
to continue in some of these cases -- so these partitions can be picked
up by grub later on and have their own entries in grub.cfg. I wondered
if the "fuseblk" override was done for a similar reason, which is why I
didn't want to change it too much. Mounted filesystems will be unmounted
later on in the script as part of `do_unmount`, so this should also
address any leftover mounts.

Let me know what you think, and thanks again for the reviews!

** Patch added: "lp1987679-plucky-v2.debdiff"
   
https://bugs.launchpad.net/ubuntu/+source/os-prober/+bug/1987679/+attachment/5853749/+files/lp1987679-plucky-v2.debdiff

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1987679

Title:
  os-prober leaves filesystems (lvm-thin, lvm snap) mounted

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/os-prober/+bug/1987679/+subscriptions


-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to