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 <nbori...@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);
>>>>
>>>
>
