* [PATCH 0/2] btrfs-progs: fix the return value of "btrfs
@ 2024-01-10 2:53 Qu Wenruo
2024-01-10 2:53 ` [PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists Qu Wenruo
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Qu Wenruo @ 2024-01-10 2:53 UTC (permalink / raw)
To: linux-btrfs
There is a bug report ("https://github.com/kdave/btrfs-progs/issues/730")
that after commit 5aa959fb3440 ("btrfs-progs: subvolume create: accept
multiple arguments"), "btrfs subvolume create" no longer properly return
1 for error cases.
It turns out that the offending commit changed how we determine the
return code, thus for several error cases, we still return 0 for
create_one_subvolume().
Fix it and add a test case for it.
Qu Wenruo (2):
btrfs-progs: cmd/subvolume: ix return value when the target exists
btrfs-progs: cli-tests: add test case for return value of "btrfs
subvlume create"
cmds/subvolume.c | 11 ++++++-
.../025-subvolume-create-failures/test.sh | 30 +++++++++++++++++++
2 files changed, 40 insertions(+), 1 deletion(-)
create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists 2024-01-10 2:53 [PATCH 0/2] btrfs-progs: fix the return value of "btrfs Qu Wenruo @ 2024-01-10 2:53 ` Qu Wenruo 2024-01-10 2:53 ` [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" Qu Wenruo 2024-01-10 3:20 ` [PATCH 0/2] btrfs-progs: fix the return value of "btrfs David Sterba 2 siblings, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2024-01-10 2:53 UTC (permalink / raw) To: linux-btrfs [BUG] When try to create a subvolume where the target path already exists, the "btrfs" comannd doesn't return error code correctly. # mkfs.btrfs -f $dev # mount $dev $mnt # touch $mnt/subv1 # btrfs subvolume create $mnt/subv1 ERROR: target path already exists: $mnt/subv1 # echo $? 0 [CAUSE] The check on whether target exists is done by path_is_dir(), if it return 0 or 1, it means there is something in that path already. But unfortunately commit 5aa959fb3440 ("btrfs-progs: subvolume create: accept multiple arguments") only changed the out tag, which would directly return @ret, not updating the return value correct. [FIX] Make sure all error out branch has their @ret manually updated. Fixes: 5aa959fb3440 ("btrfs-progs: subvolume create: accept multiple arguments") Issue: #730 Signed-off-by: Qu Wenruo <wqu@suse.com> --- cmds/subvolume.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cmds/subvolume.c b/cmds/subvolume.c index 57be9eac5e56..b01d5c80f63d 100644 --- a/cmds/subvolume.c +++ b/cmds/subvolume.c @@ -160,12 +160,14 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in } if (ret >= 0) { error("target path already exists: %s", dst); + ret = -EEXIST; goto out; } dupname = strdup(dst); if (!dupname) { error_msg(ERROR_MSG_MEMORY, "duplicating %s", dst); + ret = -ENOMEM; goto out; } newname = basename(dupname); @@ -173,18 +175,21 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in dupdir = strdup(dst); if (!dupdir) { error_msg(ERROR_MSG_MEMORY, "duplicating %s", dst); + ret = -ENOMEM; goto out; } dstdir = dirname(dupdir); if (!test_issubvolname(newname)) { error("invalid subvolume name: %s", newname); + ret = -EINVAL; goto out; } len = strlen(newname); if (len > BTRFS_VOL_NAME_MAX) { error("subvolume name too long: %s", newname); + ret = -EINVAL; goto out; } @@ -208,6 +213,8 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in goto out; } } else if (ret <= 0) { + if (ret == 0) + ret = -EEXIST; errno = ret ; error("failed to check directory %s before creation: %m", p); goto out; @@ -218,8 +225,10 @@ static int create_one_subvolume(const char *dst, struct btrfs_qgroup_inherit *in } fddst = btrfs_open_dir(dstdir, &dirstream, 1); - if (fddst < 0) + if (fddst < 0) { + ret = fddst; goto out; + } pr_verbose(LOG_DEFAULT, "Create subvolume '%s/%s'\n", dstdir, newname); if (inherit) { -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" 2024-01-10 2:53 [PATCH 0/2] btrfs-progs: fix the return value of "btrfs Qu Wenruo 2024-01-10 2:53 ` [PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists Qu Wenruo @ 2024-01-10 2:53 ` Qu Wenruo 2024-01-10 6:57 ` Qu Wenruo 2024-01-10 3:20 ` [PATCH 0/2] btrfs-progs: fix the return value of "btrfs David Sterba 2 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2024-01-10 2:53 UTC (permalink / raw) To: linux-btrfs The test case would check if "btrfs subvolume create": - Report error on an existing path - Still report error if mulitple paths are given and one of them already exists - For above case, still created a subvolume for the good parameter Signed-off-by: Qu Wenruo <wqu@suse.com> --- .../025-subvolume-create-failures/test.sh | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh new file mode 100755 index 000000000000..b268a069ba37 --- /dev/null +++ b/tests/cli-tests/025-subvolume-create-failures/test.sh @@ -0,0 +1,30 @@ +#!/bin/bash +# Create subvolume failure cases to make sure the return value is correct + +source "$TEST_TOP/common" || exit + +setup_root_helper +prepare_test_dev + +run_check_mkfs_test_dev +run_check_mount_test_dev + +# Create one subvolume and one file as place holder for later subvolume +# creation to fail. +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" +run_check $SUDO_HELPER touch "$TEST_MNT/subv2" + +# Using existing path to create a subvolume must fail +run_mustfail "should report error when target path already exists" \ + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" + +run_mustfail "should report error when target path already exists" \ + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2" + +# Using multiple subvolumes in one create go, the good one "subv3" should be +# created +run_mustfail "should report error when target path already exists" \ + $SUDO_HELPER "$TOP/btrfs" subvolume create \ + "$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3" + +run_check $SUDO_HELPER stat "$TEST_MNT/subv3" -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" 2024-01-10 2:53 ` [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" Qu Wenruo @ 2024-01-10 6:57 ` Qu Wenruo 2024-01-10 15:32 ` David Sterba 0 siblings, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2024-01-10 6:57 UTC (permalink / raw) To: linux-btrfs, David Sterba [-- Attachment #1.1.1: Type: text/plain, Size: 2143 bytes --] On 2024/1/10 13:23, Qu Wenruo wrote: > The test case would check if "btrfs subvolume create": > > - Report error on an existing path > - Still report error if mulitple paths are given and one of them already > exists > - For above case, still created a subvolume for the good parameter > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > .../025-subvolume-create-failures/test.sh | 30 +++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh > > diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh > new file mode 100755 > index 000000000000..b268a069ba37 > --- /dev/null > +++ b/tests/cli-tests/025-subvolume-create-failures/test.sh > @@ -0,0 +1,30 @@ > +#!/bin/bash > +# Create subvolume failure cases to make sure the return value is correct > + > +source "$TEST_TOP/common" || exit > + > +setup_root_helper > +prepare_test_dev > + > +run_check_mkfs_test_dev > +run_check_mount_test_dev > + > +# Create one subvolume and one file as place holder for later subvolume > +# creation to fail. > +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" > +run_check $SUDO_HELPER touch "$TEST_MNT/subv2" > + > +# Using existing path to create a subvolume must fail > +run_mustfail "should report error when target path already exists" \ > + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" > + > +run_mustfail "should report error when target path already exists" \ > + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2" > + > +# Using multiple subvolumes in one create go, the good one "subv3" should be > +# created > +run_mustfail "should report error when target path already exists" \ > + $SUDO_HELPER "$TOP/btrfs" subvolume create \ > + "$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3" > + > +run_check $SUDO_HELPER stat "$TEST_MNT/subv3" My bad, I forgot to unmount the test dev David, mind to add the following line to clean it up? run_check_umount_test_dev Thanks, Qu [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 7027 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 495 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" 2024-01-10 6:57 ` Qu Wenruo @ 2024-01-10 15:32 ` David Sterba 0 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2024-01-10 15:32 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs, David Sterba On Wed, Jan 10, 2024 at 05:27:17PM +1030, Qu Wenruo wrote: > > > On 2024/1/10 13:23, Qu Wenruo wrote: > > The test case would check if "btrfs subvolume create": > > > > - Report error on an existing path > > - Still report error if mulitple paths are given and one of them already > > exists > > - For above case, still created a subvolume for the good parameter > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > --- > > .../025-subvolume-create-failures/test.sh | 30 +++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > create mode 100755 tests/cli-tests/025-subvolume-create-failures/test.sh > > > > diff --git a/tests/cli-tests/025-subvolume-create-failures/test.sh b/tests/cli-tests/025-subvolume-create-failures/test.sh > > new file mode 100755 > > index 000000000000..b268a069ba37 > > --- /dev/null > > +++ b/tests/cli-tests/025-subvolume-create-failures/test.sh > > @@ -0,0 +1,30 @@ > > +#!/bin/bash > > +# Create subvolume failure cases to make sure the return value is correct > > + > > +source "$TEST_TOP/common" || exit > > + > > +setup_root_helper > > +prepare_test_dev > > + > > +run_check_mkfs_test_dev > > +run_check_mount_test_dev > > + > > +# Create one subvolume and one file as place holder for later subvolume > > +# creation to fail. > > +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" > > +run_check $SUDO_HELPER touch "$TEST_MNT/subv2" > > + > > +# Using existing path to create a subvolume must fail > > +run_mustfail "should report error when target path already exists" \ > > + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1" > > + > > +run_mustfail "should report error when target path already exists" \ > > + $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2" > > + > > +# Using multiple subvolumes in one create go, the good one "subv3" should be > > +# created > > +run_mustfail "should report error when target path already exists" \ > > + $SUDO_HELPER "$TOP/btrfs" subvolume create \ > > + "$TEST_MNT/subv1" "$TEST_MNT/subv2" "$TEST_MNT/subv3" > > + > > +run_check $SUDO_HELPER stat "$TEST_MNT/subv3" > > My bad, I forgot to unmount the test dev > > David, mind to add the following line to clean it up? > > run_check_umount_test_dev Updated, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] btrfs-progs: fix the return value of "btrfs 2024-01-10 2:53 [PATCH 0/2] btrfs-progs: fix the return value of "btrfs Qu Wenruo 2024-01-10 2:53 ` [PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists Qu Wenruo 2024-01-10 2:53 ` [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" Qu Wenruo @ 2024-01-10 3:20 ` David Sterba 2 siblings, 0 replies; 6+ messages in thread From: David Sterba @ 2024-01-10 3:20 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Wed, Jan 10, 2024 at 01:23:30PM +1030, Qu Wenruo wrote: > There is a bug report ("https://github.com/kdave/btrfs-progs/issues/730") > that after commit 5aa959fb3440 ("btrfs-progs: subvolume create: accept > multiple arguments"), "btrfs subvolume create" no longer properly return > 1 for error cases. > > It turns out that the offending commit changed how we determine the > return code, thus for several error cases, we still return 0 for > create_one_subvolume(). > > Fix it and add a test case for it. > > Qu Wenruo (2): > btrfs-progs: cmd/subvolume: ix return value when the target exists > btrfs-progs: cli-tests: add test case for return value of "btrfs > subvlume create" Added to devel, thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-10 15:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-01-10 2:53 [PATCH 0/2] btrfs-progs: fix the return value of "btrfs Qu Wenruo 2024-01-10 2:53 ` [PATCH 1/2] btrfs-progs: cmd/subvolume: ix return value when the target exists Qu Wenruo 2024-01-10 2:53 ` [PATCH 2/2] btrfs-progs: cli-tests: add test case for return value of "btrfs subvlume create" Qu Wenruo 2024-01-10 6:57 ` Qu Wenruo 2024-01-10 15:32 ` David Sterba 2024-01-10 3:20 ` [PATCH 0/2] btrfs-progs: fix the return value of "btrfs David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.