Code review comment for lp:~csurbhi/ubuntu/natty/os-prober/os-prober.fix764893

Revision history for this message
Colin Watson (cjwatson) wrote :

> mounted=1
> + debug "mounted as $type filesystem"

The indentation is in general wrong here - make sure to check whether
the code you're patching is using tabs or spaces for indentation, and
match it.

> + if [ $type="btrfs" ] && [ -x "$tmpmnt/@" ]; then

The first test is wrong shell syntax. This must be [ "$type" = btrfs ]
instead.

Furthermore, this won't work when we're using grub-mount, which we
generally will be nowadays; and remounting with 'mount -o ro,subvol=@ -t
btrfs' will bring us back to the bad old days before grub-mount, when we
had to choose between either corrupting hibernated filesystems or
failing to mount journalled filesystems that weren't cleanly unmounted.

I suggest instead that we check whether $type is either btrfs or fuse
(using 'case') and whether $tmpmnt/@/lib exists (this should be
reasonably safe since btrfs is only used on Linux); in that case,
bind-mount $tmpmnt/@ on $tmpmnt and remember to umount it twice at the
end. (Alternatively, we could arrange for everything to read from
$tmpmnt/@ without a bind-mount, but I think that would be rather more
code churn.)

> + warn "failed to mount" \
> + "@ ($partition)"

Try to make the warnings a bit more descriptive; most people will have
no idea what our subvolume scheme involves. They should still be
one-line warnings or thereabouts, not essays, but for example
consistently referring to "btrfs subvolume @ on $partition" would be an
improvement.

« Back to merge proposal