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!