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

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

[Nikolay Borisov]

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
+		 */
+		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);
-- 
2.7.4

[PATCH 2/2] btrfs-progs: tests: Test fs on image files is correctly recognised

[Nikolay Borisov]

This ensures that 'btrfs filesystem show' can correctly identify a
filesystem on a newly created local file.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 tests/cli-tests/010-fi-show-on-new-file/test.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100755 tests/cli-tests/010-fi-show-on-new-file/test.sh

diff --git a/tests/cli-tests/010-fi-show-on-new-file/test.sh b/tests/cli-tests/010-fi-show-on-new-file/test.sh
new file mode 100755
index 000000000000..668f0f9b2cfe
--- /dev/null
+++ b/tests/cli-tests/010-fi-show-on-new-file/test.sh
@@ -0,0 +1,16 @@
+#!/bin/bash
+# test for 'filesystem show' on fresh local file
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+IMAGE=$(mktemp -u btrfs-XXXXXX.img)
+
+truncate -s3g "$IMAGE"
+run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$IMAGE"
+run_check $SUDO_HELPER "$TOP/btrfs" filesystem show "$IMAGE"
+
+rm -f "$IMAGE"
+
-- 
2.7.4

Re: [PATCH 2/2] btrfs-progs: tests: Test fs on image files is correctly recognised

[David Sterba]

On Thu, May 16, 2019 at 04:12:50PM +0300, Nikolay Borisov wrote:
> This ensures that 'btrfs filesystem show' can correctly identify a
> filesystem on a newly created local file.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  tests/cli-tests/010-fi-show-on-new-file/test.sh | 16 ++++++++++++++++

Test is ok, the placment should be to misc-tests, cli-tests is really to
verify that argumets get parsed and that some combinations are handled.

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

[Qu Wenruo]

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);

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);
>

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

[Nikolay Borisov]

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.

> 
> 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);
>>
> 

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

[Qu Wenruo]

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:

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);
>>>
>>

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

[Nikolay Borisov]

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);
>>>>
>>>
> 

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

[Qu Wenruo]

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 <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.

Fair enough.

Reviewed-by: Qu Wenruo <wqu@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);
>>>>>
>>>>
>>

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

[David Sterba]

On Thu, May 16, 2019 at 04:12:49PM +0300, 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>

Applied, thanks.