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

[Boris Burkov <boris@bur.io>]

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