Re: [PATCH 1/2] btrfs-progs: Correctly open filesystem on image file

[Nikolay Borisov <nborisov@suse.com>]

On 16.05.19 г. 16:54 ч., Qu Wenruo wrote:
> 
> 
> On 2019/5/16 下午9:45, Nikolay Borisov wrote:
>>
>>
>> On 16.05.19 г. 16:41 ч., Qu Wenruo wrote:
>>>
>>>
>>> On 2019/5/16 下午9:12, Nikolay Borisov wrote:
>>>> When btrfs' 'filesystem' subcommand is passed path to an image file it
>>>> currently fails since the code expects the image file is going to be
>>>> recognised by libblkid (called from btrfs_scan_devices()). This is not
>>>> the case since libblkid only scan well-known locations under /dev.
>>>>
>>>> Fix this by explicitly calling open_ctree which will correctly open
>>>> the image and add it to the correct btrfs_fs_devices struct. This allows
>>>> subsequent cmd_filesystem_show logic to correctly show requested
>>>> information.
>>>>
>>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>>> ---
>>>>  cmds-filesystem.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
>>>> index b8beec13f0e5..f55ce9b4ab85 100644
>>>> --- a/cmds-filesystem.c
>>>> +++ b/cmds-filesystem.c
>>>> @@ -771,7 +771,18 @@ static int cmd_filesystem_show(int argc, char **argv)
>>>>  		goto out;
>>>>
>>>>  devs_only:
>>>> -	ret = btrfs_scan_devices();
>>>> +	if (type == BTRFS_ARG_REG) {
>>>> +		/*
>>>> +		 * We don't close the fs_info because it will free the device,
>>>> +		 * this is not a long-running process so it's fine
>>>> +		 */
>>>
>>> The comment makes sense, but I'm pretty sure we still prefer to clean it up.
>>>
>>> Just something like:
>>>
>>> 	struct btrfs_root *root = NULL;
>>>
>>> 	root = open_ctree();
>>> 	if (root)
>>> 		ret = 0;
>>> 	else
>>> 		ret = 1;
>>> 	close_ctree(root);
>>
>> Tested that and it doesn't work, because close_ctree will call
>> btrfs_close_devices which frees existing devices so the test fails.
> 
> Sorry for the confusion, I missed "..." line before close_ctree(root);
> 
> I mean something like:

That would work but it quickly becomes ugly due to the other possible
failures i.e :

 ret = map_seed_devices(&all_uuids);
        if (ret) {

                error("mapping seed devices returned error %d", ret);

                return 1;

        }

It's not critical since we return 1 and all allocated memory is free. So
the options is to either change those 'return 1' statement to 'goto
close_ctree' or just leave it as is currently. I opted for the the latter.

> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index c4e43f8446dd..d20cbd49c201 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -677,6 +677,7 @@ static int cmd_filesystem_show(int argc, char **argv)
>  {
>         LIST_HEAD(all_uuids);
>         struct btrfs_fs_devices *fs_devices;
> +       struct btrfs_root *root = NULL;
>         char *search = NULL;
>         int ret;
>         /* default, search both kernel and udev */
> @@ -779,7 +780,8 @@ devs_only:
>                  * We don't close the fs_info because it will free the
> device,
>                  * this is not a long-running process so it's fine
>                  */
> -               if (open_ctree(search, btrfs_sb_offset(0), 0))
> +               root = open_ctree(search, 0, 0);
> +               if (root)
>                         ret = 0;
>                 else
>                         ret = 1;
> @@ -821,6 +823,7 @@ devs_only:
>                 free_fs_devices(fs_devices);
>         }
>  out:
> +       close_ctree(root);
>         free_seen_fsid(seen_fsid_hash);
>         return ret;
>  }
> 
> Thanks,
> Qu
>>
>>>
>>> Despite that, I think the patch looks good to me.
>>>
>>> Thanks,
>>> Qu
>>>
>>>> +		if (open_ctree(search, btrfs_sb_offset(0), 0))
>>>> +			ret = 0;
>>>> +		else
>>>> +			ret = 1;
>>>> +	} else {
>>>> +		ret = btrfs_scan_devices();
>>>> +	}
>>>>
>>>>  	if (ret) {
>>>>  		error("blkid device scan returned %d", ret);
>>>>
>>>
>