On 2019/5/16 下午10:01, Nikolay Borisov wrote:
>
>
> 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.
Fair enough.
Reviewed-by: Qu Wenruo <w...@suse.com>
The problem can be later solved by implementing refcount for each
device, but so far we don't have so much desire for that feature, so it
should be OK.
Thanks,
Qu
>
>>
>> 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);
>>>>>
>>>>
>>
