[PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Qu Wenruo]

[BUG]
With the latest kernel patch to reject invalid qgroupids in
btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
subvolume snapshot" can lead to the following output:

 # mkfs.btrfs -O quota -f $dev
 # mount $dev $mnt
 # btrfs subvolume create -i 2/0 $mnt/subv1
 Create subvolume '/mnt/btrfs/subv1'
 ERROR: cannot create subvolume: No such file or directory

The "btrfs subvolume" command output the first line, seemingly to
indicate a successful subvolume creation, then followed by an error
message.

This can be a little confusing on whether if the subvolume is created or
not.

[FIX]
Fix the output by only outputting the regular line if the ioctl
succeeded.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/subvolume.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 00c5eacfa694..1549adaca642 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -229,7 +229,6 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 		goto out;
 	}
 
-	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
 	if (inherit) {
 		struct btrfs_ioctl_vol_args_v2	args;
 
@@ -253,6 +252,7 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
 		error("cannot create subvolume: %m");
 		goto out;
 	}
+	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
 
 out:
 	close(fddst);
@@ -754,16 +754,8 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 	if (fd < 0)
 		goto out;
 
-	if (readonly) {
+	if (readonly)
 		args.flags |= BTRFS_SUBVOL_RDONLY;
-		pr_verbose(LOG_DEFAULT,
-			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);
-	} else {
-		pr_verbose(LOG_DEFAULT,
-			   "Create a snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);
-	}
 
 	args.fd = fd;
 	if (inherit) {
@@ -784,6 +776,15 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
 
 	retval = 0;	/* success */
 
+	if (readonly)
+		pr_verbose(LOG_DEFAULT,
+			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
+			   subvol, dstdir, newname);
+	else
+		pr_verbose(LOG_DEFAULT,
+			   "Create a snapshot of '%s' in '%s/%s'\n",
+			   subvol, dstdir, newname);
+
 out:
 	close(fddst);
 	close(fd);
-- 
2.43.2

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[HAN Yuwei]

[-- Attachment #1.1.1: Type: text/plain, Size: 2694 bytes --]

> [BUG]
> With the latest kernel patch to reject invalid qgroupids in
> btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> subvolume snapshot" can lead to the following output:
>
>   # mkfs.btrfs -O quota -f $dev
>   # mount $dev $mnt
>   # btrfs subvolume create -i 2/0 $mnt/subv1
>   Create subvolume '/mnt/btrfs/subv1'
>   ERROR: cannot create subvolume: No such file or directory
>
> The "btrfs subvolume" command output the first line, seemingly to
> indicate a successful subvolume creation, then followed by an error
> message.
>
> This can be a little confusing on whether if the subvolume is created or
> not.
>
> [FIX]
> Fix the output by only outputting the regular line if the ioctl
> succeeded.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   cmds/subvolume.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index 00c5eacfa694..1549adaca642 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -229,7 +229,6 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
>   		goto out;
>   	}
>   
> -	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
>   	if (inherit) {
>   		struct btrfs_ioctl_vol_args_v2	args;
>   
> @@ -253,6 +252,7 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in
>   		error("cannot create subvolume: %m");
>   		goto out;
>   	}
> +	pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname);
>   


How about saying "Created subvolume %s/%s" ?


>   out:
>   	close(fddst);
> @@ -754,16 +754,8 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
>   	if (fd < 0)
>   		goto out;
>   
> -	if (readonly) {
> +	if (readonly)
>   		args.flags |= BTRFS_SUBVOL_RDONLY;
> -		pr_verbose(LOG_DEFAULT,
> -			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> -	} else {
> -		pr_verbose(LOG_DEFAULT,
> -			   "Create a snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> -	}
>   
>   	args.fd = fd;
>   	if (inherit) {
> @@ -784,6 +776,15 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *
>   
>   	retval = 0;	/* success */
>   
> +	if (readonly)
> +		pr_verbose(LOG_DEFAULT,
> +			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
> +			   subvol, dstdir, newname);
> +	else
> +		pr_verbose(LOG_DEFAULT,
> +			   "Create a snapshot of '%s' in '%s/%s'\n",
> +			   subvol, dstdir, newname);
> +
>   out:
>   	close(fddst);
>   	close(fd);

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 1589 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Qu Wenruo]

在 2024/2/27 15:29, HAN Yuwei 写道:
>> [BUG]
>> With the latest kernel patch to reject invalid qgroupids in
>> btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
>> subvolume snapshot" can lead to the following output:
>>
>>   # mkfs.btrfs -O quota -f $dev
>>   # mount $dev $mnt
>>   # btrfs subvolume create -i 2/0 $mnt/subv1
>>   Create subvolume '/mnt/btrfs/subv1'
>>   ERROR: cannot create subvolume: No such file or directory
>>
>> The "btrfs subvolume" command output the first line, seemingly to
>> indicate a successful subvolume creation, then followed by an error
>> message.
>>
>> This can be a little confusing on whether if the subvolume is created or
>> not.
>>
>> [FIX]
>> Fix the output by only outputting the regular line if the ioctl
>> succeeded.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   cmds/subvolume.c | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
>> index 00c5eacfa694..1549adaca642 100644
>> --- a/cmds/subvolume.c
>> +++ b/cmds/subvolume.c
>> @@ -229,7 +229,6 @@ static int create_one_subvolume(const char *dst, 
>> struct btrfs_qgroup_inherit *in
>>           goto out;
>>       }
>> -    pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, 
>> newname);
>>       if (inherit) {
>>           struct btrfs_ioctl_vol_args_v2    args;
>> @@ -253,6 +252,7 @@ static int create_one_subvolume(const char *dst, 
>> struct btrfs_qgroup_inherit *in
>>           error("cannot create subvolume: %m");
>>           goto out;
>>       }
>> +    pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, 
>> newname);
> 
> 
> How about saying "Created subvolume %s/%s" ?

Sounds pretty reasonable, would go that way in the next update.

Thanks,
Qu
> 
> 
>>   out:
>>       close(fddst);
>> @@ -754,16 +754,8 @@ static int cmd_subvolume_snapshot(const struct 
>> cmd_struct *cmd, int argc, char *
>>       if (fd < 0)
>>           goto out;
>> -    if (readonly) {
>> +    if (readonly)
>>           args.flags |= BTRFS_SUBVOL_RDONLY;
>> -        pr_verbose(LOG_DEFAULT,
>> -               "Create a readonly snapshot of '%s' in '%s/%s'\n",
>> -               subvol, dstdir, newname);
>> -    } else {
>> -        pr_verbose(LOG_DEFAULT,
>> -               "Create a snapshot of '%s' in '%s/%s'\n",
>> -               subvol, dstdir, newname);
>> -    }
>>       args.fd = fd;
>>       if (inherit) {
>> @@ -784,6 +776,15 @@ static int cmd_subvolume_snapshot(const struct 
>> cmd_struct *cmd, int argc, char *
>>       retval = 0;    /* success */
>> +    if (readonly)
>> +        pr_verbose(LOG_DEFAULT,
>> +               "Create a readonly snapshot of '%s' in '%s/%s'\n",
>> +               subvol, dstdir, newname);
>> +    else
>> +        pr_verbose(LOG_DEFAULT,
>> +               "Create a snapshot of '%s' in '%s/%s'\n",
>> +               subvol, dstdir, newname);
>> +
>>   out:
>>       close(fddst);
>>       close(fd);

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[David Sterba]

On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
> [BUG]
> With the latest kernel patch to reject invalid qgroupids in
> btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> subvolume snapshot" can lead to the following output:
> 
>  # mkfs.btrfs -O quota -f $dev
>  # mount $dev $mnt
>  # btrfs subvolume create -i 2/0 $mnt/subv1
>  Create subvolume '/mnt/btrfs/subv1'
>  ERROR: cannot create subvolume: No such file or directory
> 
> The "btrfs subvolume" command output the first line, seemingly to
> indicate a successful subvolume creation, then followed by an error
> message.
> 
> This can be a little confusing on whether if the subvolume is created or
> not.
> 
> [FIX]
> Fix the output by only outputting the regular line if the ioctl
> succeeded.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to devel, thanks.

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Boris Burkov]

On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
> On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
> > [BUG]
> > With the latest kernel patch to reject invalid qgroupids in
> > btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> > subvolume snapshot" can lead to the following output:
> > 
> >  # mkfs.btrfs -O quota -f $dev
> >  # mount $dev $mnt
> >  # btrfs subvolume create -i 2/0 $mnt/subv1
> >  Create subvolume '/mnt/btrfs/subv1'
> >  ERROR: cannot create subvolume: No such file or directory
> > 
> > The "btrfs subvolume" command output the first line, seemingly to
> > indicate a successful subvolume creation, then followed by an error
> > message.
> > 
> > This can be a little confusing on whether if the subvolume is created or
> > not.
> > 
> > [FIX]
> > Fix the output by only outputting the regular line if the ioctl
> > succeeded.
> > 
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Added to devel, thanks.

This patch breaks every test that creates snapshots or subvolumes and
expects the output in the outfile.

That's because it did:
s/Create a snapshot/Create snapshot/

Please run the tests when making changes! This failed on btrfs/001, so
it would have taken 1 second to see.

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Qu Wenruo]

在 2024/3/27 06:53, Boris Burkov 写道:
> On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
>> On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
>>> [BUG]
>>> With the latest kernel patch to reject invalid qgroupids in
>>> btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
>>> subvolume snapshot" can lead to the following output:
>>>
>>>   # mkfs.btrfs -O quota -f $dev
>>>   # mount $dev $mnt
>>>   # btrfs subvolume create -i 2/0 $mnt/subv1
>>>   Create subvolume '/mnt/btrfs/subv1'
>>>   ERROR: cannot create subvolume: No such file or directory
>>>
>>> The "btrfs subvolume" command output the first line, seemingly to
>>> indicate a successful subvolume creation, then followed by an error
>>> message.
>>>
>>> This can be a little confusing on whether if the subvolume is created or
>>> not.
>>>
>>> [FIX]
>>> Fix the output by only outputting the regular line if the ioctl
>>> succeeded.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> Added to devel, thanks.
> 
> This patch breaks every test that creates snapshots or subvolumes and
> expects the output in the outfile.
> 
> That's because it did:
> s/Create a snapshot/Create snapshot/
> 
> Please run the tests when making changes! This failed on btrfs/001, so
> it would have taken 1 second to see.

Wrong patch to blame?

The message is kept the same in the patch:

-		pr_verbose(LOG_DEFAULT,
-			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);
-		pr_verbose(LOG_DEFAULT,
-			   "Create a snapshot of '%s' in '%s/%s'\n",
-			   subvol, dstdir, newname);

+		pr_verbose(LOG_DEFAULT,
+			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
+			   subvol, dstdir, newname);
+		pr_verbose(LOG_DEFAULT,
+			   "Create a snapshot of '%s' in '%s/%s'\n",
+			   subvol, dstdir, newname);

Thanks,
Qu

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Qu Wenruo]

在 2024/3/27 06:57, Qu Wenruo 写道:
> 
> 
> 在 2024/3/27 06:53, Boris Burkov 写道:
>> On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
>>> On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
>>>> [BUG]
>>>> With the latest kernel patch to reject invalid qgroupids in
>>>> btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
>>>> subvolume snapshot" can lead to the following output:
>>>>
>>>>   # mkfs.btrfs -O quota -f $dev
>>>>   # mount $dev $mnt
>>>>   # btrfs subvolume create -i 2/0 $mnt/subv1
>>>>   Create subvolume '/mnt/btrfs/subv1'
>>>>   ERROR: cannot create subvolume: No such file or directory
>>>>
>>>> The "btrfs subvolume" command output the first line, seemingly to
>>>> indicate a successful subvolume creation, then followed by an error
>>>> message.
>>>>
>>>> This can be a little confusing on whether if the subvolume is 
>>>> created or
>>>> not.
>>>>
>>>> [FIX]
>>>> Fix the output by only outputting the regular line if the ioctl
>>>> succeeded.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>
>>> Added to devel, thanks.
>>
>> This patch breaks every test that creates snapshots or subvolumes and
>> expects the output in the outfile.
>>
>> That's because it did:
>> s/Create a snapshot/Create snapshot/
>>
>> Please run the tests when making changes! This failed on btrfs/001, so
>> it would have taken 1 second to see.
> 
> Wrong patch to blame?
> 
> The message is kept the same in the patch:
> 
> -        pr_verbose(LOG_DEFAULT,
> -               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> -               subvol, dstdir, newname);
> -        pr_verbose(LOG_DEFAULT,
> -               "Create a snapshot of '%s' in '%s/%s'\n",
> -               subvol, dstdir, newname);
> 
> +        pr_verbose(LOG_DEFAULT,
> +               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> +               subvol, dstdir, newname);
> +        pr_verbose(LOG_DEFAULT,
> +               "Create a snapshot of '%s' in '%s/%s'\n",
> +               subvol, dstdir, newname);
> 
> Thanks,
> Qu

OK, David seems to changed the output line when merging the patch...

That's something out of my reach.

Thanks,
Qu

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Boris Burkov]

On Wed, Mar 27, 2024 at 07:00:29AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 06:57, Qu Wenruo 写道:
> > 
> > 
> > 在 2024/3/27 06:53, Boris Burkov 写道:
> > > On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
> > > > On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
> > > > > [BUG]
> > > > > With the latest kernel patch to reject invalid qgroupids in
> > > > > btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> > > > > subvolume snapshot" can lead to the following output:
> > > > > 
> > > > >   # mkfs.btrfs -O quota -f $dev
> > > > >   # mount $dev $mnt
> > > > >   # btrfs subvolume create -i 2/0 $mnt/subv1
> > > > >   Create subvolume '/mnt/btrfs/subv1'
> > > > >   ERROR: cannot create subvolume: No such file or directory
> > > > > 
> > > > > The "btrfs subvolume" command output the first line, seemingly to
> > > > > indicate a successful subvolume creation, then followed by an error
> > > > > message.
> > > > > 
> > > > > This can be a little confusing on whether if the subvolume
> > > > > is created or
> > > > > not.
> > > > > 
> > > > > [FIX]
> > > > > Fix the output by only outputting the regular line if the ioctl
> > > > > succeeded.
> > > > > 
> > > > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > > 
> > > > Added to devel, thanks.
> > > 
> > > This patch breaks every test that creates snapshots or subvolumes and
> > > expects the output in the outfile.
> > > 
> > > That's because it did:
> > > s/Create a snapshot/Create snapshot/
> > > 
> > > Please run the tests when making changes! This failed on btrfs/001, so
> > > it would have taken 1 second to see.
> > 
> > Wrong patch to blame?
> > 
> > The message is kept the same in the patch:
> > 
> > -        pr_verbose(LOG_DEFAULT,
> > -               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> > -               subvol, dstdir, newname);
> > -        pr_verbose(LOG_DEFAULT,
> > -               "Create a snapshot of '%s' in '%s/%s'\n",
> > -               subvol, dstdir, newname);
> > 
> > +        pr_verbose(LOG_DEFAULT,
> > +               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> > +        pr_verbose(LOG_DEFAULT,
> > +               "Create a snapshot of '%s' in '%s/%s'\n",
> > +               subvol, dstdir, newname);
> > 
> > Thanks,
> > Qu
> 
> OK, David seems to changed the output line when merging the patch...
> 
> That's something out of my reach.

Agreed. Sorry about the test rant.

> 
> Thanks,
> Qu

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[David Sterba]

On Tue, Mar 26, 2024 at 01:34:43PM -0700, Boris Burkov wrote:
> On Wed, Mar 27, 2024 at 07:00:29AM +1030, Qu Wenruo wrote:
> > 
> > 
> > 在 2024/3/27 06:57, Qu Wenruo 写道:
> > > 
> > > 
> > > 在 2024/3/27 06:53, Boris Burkov 写道:
> > > > On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
> > > > > On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
> > > > > > [BUG]
> > > > > > With the latest kernel patch to reject invalid qgroupids in
> > > > > > btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> > > > > > subvolume snapshot" can lead to the following output:
> > > > > > 
> > > > > >   # mkfs.btrfs -O quota -f $dev
> > > > > >   # mount $dev $mnt
> > > > > >   # btrfs subvolume create -i 2/0 $mnt/subv1
> > > > > >   Create subvolume '/mnt/btrfs/subv1'
> > > > > >   ERROR: cannot create subvolume: No such file or directory
> > > > > > 
> > > > > > The "btrfs subvolume" command output the first line, seemingly to
> > > > > > indicate a successful subvolume creation, then followed by an error
> > > > > > message.
> > > > > > 
> > > > > > This can be a little confusing on whether if the subvolume
> > > > > > is created or
> > > > > > not.
> > > > > > 
> > > > > > [FIX]
> > > > > > Fix the output by only outputting the regular line if the ioctl
> > > > > > succeeded.
> > > > > > 
> > > > > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > > > 
> > > > > Added to devel, thanks.
> > > > 
> > > > This patch breaks every test that creates snapshots or subvolumes and
> > > > expects the output in the outfile.
> > > > 
> > > > That's because it did:
> > > > s/Create a snapshot/Create snapshot/
> > > > 
> > > > Please run the tests when making changes! This failed on btrfs/001, so
> > > > it would have taken 1 second to see.
> > > 
> > > Wrong patch to blame?
> > > 
> > > The message is kept the same in the patch:
> > > 
> > > -        pr_verbose(LOG_DEFAULT,
> > > -               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> > > -               subvol, dstdir, newname);
> > > -        pr_verbose(LOG_DEFAULT,
> > > -               "Create a snapshot of '%s' in '%s/%s'\n",
> > > -               subvol, dstdir, newname);
> > > 
> > > +        pr_verbose(LOG_DEFAULT,
> > > +               "Create a readonly snapshot of '%s' in '%s/%s'\n",
> > > +               subvol, dstdir, newname);
> > > +        pr_verbose(LOG_DEFAULT,
> > > +               "Create a snapshot of '%s' in '%s/%s'\n",
> > > +               subvol, dstdir, newname);
> > > 
> > > Thanks,
> > > Qu
> > 
> > OK, David seems to changed the output line when merging the patch...
> > 
> > That's something out of my reach.
> 
> Agreed. Sorry about the test rant.

I changed it so the message is unified with others, Anand promised to
add/fix the fstests filter. What fstests does is fragile and this is
unfortunatelly not the last time this happens.

Re: [PATCH] btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded

[Boris Burkov]

On Wed, Mar 27, 2024 at 06:57:48AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 06:53, Boris Burkov 写道:
> > On Fri, Mar 01, 2024 at 01:56:31PM +0100, David Sterba wrote:
> > > On Tue, Feb 27, 2024 at 02:41:16PM +1030, Qu Wenruo wrote:
> > > > [BUG]
> > > > With the latest kernel patch to reject invalid qgroupids in
> > > > btrfs_qgroup_inherit structure, "btrfs subvolume create" or "btrfs
> > > > subvolume snapshot" can lead to the following output:
> > > > 
> > > >   # mkfs.btrfs -O quota -f $dev
> > > >   # mount $dev $mnt
> > > >   # btrfs subvolume create -i 2/0 $mnt/subv1
> > > >   Create subvolume '/mnt/btrfs/subv1'
> > > >   ERROR: cannot create subvolume: No such file or directory
> > > > 
> > > > The "btrfs subvolume" command output the first line, seemingly to
> > > > indicate a successful subvolume creation, then followed by an error
> > > > message.
> > > > 
> > > > This can be a little confusing on whether if the subvolume is created or
> > > > not.
> > > > 
> > > > [FIX]
> > > > Fix the output by only outputting the regular line if the ioctl
> > > > succeeded.
> > > > 
> > > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > 
> > > Added to devel, thanks.
> > 
> > This patch breaks every test that creates snapshots or subvolumes and
> > expects the output in the outfile.
> > 
> > That's because it did:
> > s/Create a snapshot/Create snapshot/
> > 
> > Please run the tests when making changes! This failed on btrfs/001, so
> > it would have taken 1 second to see.
> 
> Wrong patch to blame?
> 
> The message is kept the same in the patch:
> 
> -		pr_verbose(LOG_DEFAULT,
> -			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> -		pr_verbose(LOG_DEFAULT,
> -			   "Create a snapshot of '%s' in '%s/%s'\n",
> -			   subvol, dstdir, newname);
> 
> +		pr_verbose(LOG_DEFAULT,
> +			   "Create a readonly snapshot of '%s' in '%s/%s'\n",
> +			   subvol, dstdir, newname);
> +		pr_verbose(LOG_DEFAULT,
> +			   "Create a snapshot of '%s' in '%s/%s'\n",
> +			   subvol, dstdir, newname);
> 
> Thanks,
> Qu

Hmm, something weird happened. I'm looking at commit
5f87b467a9e7 ("btrfs-progs: subvolume: output the prompt line only when the ioctl succeeded")
in the master branch of https://github.com/kdave/btrfs-progs.git

and it has the (relevant) diff:

-       if (readonly) {
+       if (readonly)
                args.flags |= BTRFS_SUBVOL_RDONLY;
-               pr_verbose(LOG_DEFAULT,
-                          "Create a readonly snapshot of '%s' in '%s/%s'\n",
-                          subvol, dstdir, newname);
-       } else {
-               pr_verbose(LOG_DEFAULT,
-                          "Create a snapshot of '%s' in '%s/%s'\n",
-                          subvol, dstdir, newname);
-       }

        args.fd = fd;
        if (inherit) {
@@ -784,6 +776,15 @@ static int cmd_subvolume_snapshot(const struct cmd_struct *cmd, int argc, char *

        retval = 0;     /* success */

+       if (readonly)
+               pr_verbose(LOG_DEFAULT,
+                          "Create readonly snapshot of '%s' in '%s/%s'\n",
+                          subvol, dstdir, newname);
+       else
+               pr_verbose(LOG_DEFAULT,
+                          "Create snapshot of '%s' in '%s/%s'\n",
+                          subvol, dstdir, newname);
+