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

[David Sterba <dsterba@suse.cz>]

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.