Re: [PATCH] Btrfs-progs: receive: fix the case that we can not find subvolume

[Michael Welsh Duggan <mwd@cert.org>]

David Sterba <dsterba@suse.cz> writes:

> On Tue, Dec 17, 2013 at 05:13:49PM +0800, Wang Shilong wrote:
>> If we change our default subvolume, btrfs receive will fail to find
>> subvolume. To fix this problem, i have two ideas.
>> 
>> 1.make btrfs snapshot ioctl support passing source subvolume's objectid
>
>> 2.when we want to using interval subvolume path, we mount it other place
>> that use subvolume 5 as its default subvolume.
>
> 3. Tell the user to mount the toplevel subvol by himself and run receive
>    again

Ugh.  I hope that would be considered a short-term hack waiting for a
better solution, perhaps requiring a kernel upgrade.  From a user's
perspective there is no reason this should be necessary, and requiring
this would be extraordinarily surprising.  "Why is btrfs unable to find
my snapshot?  It's right there!"  Moreover, this used to work just fine
in previous versions of btrfs-progs.

>> We'd better use the second approach because it won't bother kernel change.
>
> I don't think that the silent mount is the right way to fix it, that way
> the btrfs tool tooks responsibility not to break anything.  Like the
> unhandled umount failure below. I think admins and power users do not
> like to see some random tool mess with the system like this.
>
>> @@ -199,6 +200,10 @@ static int process_snapshot(const char *path,
>> const u8 *uuid, u64 ctransid,
>>  	char uuid_str[BTRFS_UUID_UNPARSED_SIZE];
>>  	struct btrfs_ioctl_vol_args_v2 args_v2;
>>  	struct subvol_info *parent_subvol = NULL;
>> +	char *dev = NULL;
>> +	char tmp_name[15] = "btrfs-XXXXXX";
>> +	char tmp_dir[30] = "/tmp";
>
> Mounting valuable data under /tmp is dangerous, what if some /tmp
> cleaner starts to remove old files. I've seen that happen in practice.

Agreed.  If you _were_ to continue to implement it like this, you should
include code to respect the TMPDIR envvar at the very least.

-- 
Michael Welsh Duggan
(mwd@cert.org)