Code review comment for lp:~csurbhi/partman-btrfs/testing-subvol

Could you propose this for merging into
lp:~ubuntu-core-dev/partman-btrfs/ubuntu instead, rather than the
upstream lp:partman-btrfs? That way I won't see the changes from
5ubuntu1 in the review diff.

On Fri, Jan 28, 2011 at 02:14:14PM -0000, Surbhi Palande wrote:
> Code:
> 1) fstab.d/btrfs checks if /home is a separate partition or not. If / is btrfs formatted then it adds an entry to mount the subvol=@home on /home even if /home is not a separate partition.
> 2) In case /home is a separate btrfs partition then it adds an entry to mount the subvolume=@home on /home.
> 3) Only when both / and /home are not btrfs formatted, then no entry corresponding to @home is added.

Your general approach looks fine to me.

> if [ "$mountpoint" = / ]; then
> + if [ $home_found = unknown ]; then

In shell, a good rule of thumb is to always put "" around any word
containing $ unless there's a specific reason not to. It doesn't matter
in practice here because the value of home_found is always a single
word, but it would be good style to use "$home_found" instead (and
similar for other $-expansions in your branch). I know this guideline
isn't followed universally in the installer, but I'd like to see at
least new code following it.

(Good reason not to put "" around a word containing $ are when you
explicitly want word splitting to be performed, or in the first argument
to 'case' which is never word-split so doesn't require quoting.)

More generally, I think there may be an ordering problem here. You're
iterating in disk/partition order, so this will go wrong if it happens
that the /home partition is on an earlier disk or partition than /.
This probably won't be the common case but we should still deal with it.

I suggest that you should make this file have two loops, both with
roughly the same form (don't worry about optimising away multiple calls
to 'open_dialog PARTITIONS' etc. - this code is run a few times, but it
isn't particularly time-critical). The first should just check whether
there's a partition with mountpoint=/home. The second should be more or
less what was there before your change, but with the extra changes to
options and home_options. That way, it will work whichever way round
the partitions happen to be.

> - mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
> + options=${options%,subvol=*}
> + options=${options:+"-o $options"}

I'd find it clearer to put the quotes around the whole $-expansion here
(and similarly below), i.e.:

  options="${options:+-o $options}"

This means you don't have to wonder whether it's quoted that way because
the "" are supposed to end up in the value of options.

> + mkdir -p /target$mp

You can skip this - mount.d scripts are called from
partman-target/finish.d/mount_partitions, which always creates the
directory first.

> + mount -t btrfs $options $fs /target$mp || exit 1
> + case $mp in
> + /)
> + btrfs subvolume create /target$mp/@
> + umount /target$mp
> + options=${options:+"$options,subvol=@"}
> + options=${options:="-o subvol=@"}

Using := here is odd. "${options:=-o subvol=@}" assigns to options, and
then you assign the result of that to options again ...

The repeated expansions here have become a little difficult to visually
unpack, and I suspect that adding -o to the start of options at the top
contributes to the problem. How about something like this instead
(untested, though)?

  mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
  case $mp in
      /)
          btrfs subvolume create "/target$mp/@"
          umount "/target$mp"
          options="${options:+$options,}subvol=@"
          mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
          ;;
      /home)
          btrfs subvolume create "/target$mp/@home"
          umount "/target$mp"
          options="${options:+$options,}subvol=@home"
          mount -t btrfs ${options:+-o "$options"} $fs /target$mp || exit 1
          ;;
  esac

I think once these issues are fixed it should be good to go. Thanks!