[devel] btrfs-progs: subvolume: Implement delete --subvolid <path>
diff mbox series

Message ID 20191028160506.22245-1-marcos.souza.org@gmail.com
State New
Headers show
Series
  • [devel] btrfs-progs: subvolume: Implement delete --subvolid <path>
Related show

Commit Message

Marcos Paulo de Souza Oct. 28, 2019, 4:05 p.m. UTC
From: Marcos Paulo de Souza <mpdesouza@suse.com>

With this patch a user can delete a subvolume from a btrfs using only
the subvolume id in the format

btrfs subvolume delete --subvolid <volid> <path>

Fixes: #152

Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 Documentation/btrfs-subvolume.asciidoc        | 10 ++++-
 cmds/subvolume.c                              | 39 +++++++++++++++++--
 tests/misc-tests/038-delete-subvolume/test.sh | 30 ++++++++++++++
 3 files changed, 75 insertions(+), 4 deletions(-)
 create mode 100755 tests/misc-tests/038-delete-subvolume/test.sh

Comments

Anand Jain Oct. 29, 2019, 2:05 a.m. UTC | #1
On 29/10/19 12:05 AM, Marcos Paulo de Souza wrote:
> From: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
> With this patch a user can delete a subvolume from a btrfs using only
> the subvolume id in the format
> 
> btrfs subvolume delete --subvolid <volid> <path>
> 
> Fixes: #152
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> ---
>   Documentation/btrfs-subvolume.asciidoc        | 10 ++++-
>   cmds/subvolume.c                              | 39 +++++++++++++++++--
>   tests/misc-tests/038-delete-subvolume/test.sh | 30 ++++++++++++++
>   3 files changed, 75 insertions(+), 4 deletions(-)
>   create mode 100755 tests/misc-tests/038-delete-subvolume/test.sh
> 
> diff --git a/Documentation/btrfs-subvolume.asciidoc b/Documentation/btrfs-subvolume.asciidoc
> index 49c72e89..9492fc19 100644
> --- a/Documentation/btrfs-subvolume.asciidoc
> +++ b/Documentation/btrfs-subvolume.asciidoc
> @@ -59,12 +59,17 @@ directory.
>   Add the newly created subvolume to a qgroup. This option can be given multiple
>   times.
>   
> -*delete* [options] <subvolume> [<subvolume>...]::
> +*delete* [options] <subvolume> [<subvolume>...]
> +
> +*delete* [options] [--subvolid <subvolid> <path>]::
>   Delete the subvolume(s) from the filesystem.
>   +
>   If <subvolume> is not a subvolume, btrfs returns an error but continues if
>   there are more arguments to process.
>   +
> +If --subvolid if used, <path> must point to a btrfs filesystem. See `btrfs
> +subvolume list` how to get the subvolume id.
> ++
>   The corresponding directory is removed instantly but the data blocks are
>   removed later in the background. The command returns immediately. See `btrfs
>   subvolume sync` how to wait until the subvolume gets completely removed.
> @@ -84,6 +89,9 @@ wait for transaction commit after deleting each subvolume.
>   +
>   -v|--verbose::::
>   verbose output of operations.
> ++
> +-s|--subvolid::::
> +subvolume id of the to be removed subvolume from <path>
>   
>   *find-new* <subvolume> <last_gen>::
>   List the recently modified files in a subvolume, after <last_gen> generation.
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index 7a5fd79b..df75f82d 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -223,6 +223,7 @@ static int wait_for_commit(int fd)
>   
>   static const char * const cmd_subvol_delete_usage[] = {
>   	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
> +	"btrfs subvolume delete [options] [--subvolid <subvolid> <path>]",


It should rather be..

+ "btrfs subvolume delete [options] <[-s|--subvolid <subvolid> <path>] | 
[<subvolume>...]>"

Which means..
  either -s|--subvolid <subvolid> OR the <subvolume> is mandatory option.


>   	"Delete subvolume(s)",
>   	"Delete subvolumes from the filesystem. The corresponding directory",
>   	"is removed instantly but the data blocks are removed later.",
> @@ -234,6 +235,7 @@ static const char * const cmd_subvol_delete_usage[] = {
>   	"-c|--commit-after      wait for transaction commit at the end of the operation",
>   	"-C|--commit-each       wait for transaction commit after deleting each subvolume",
>   	"-v|--verbose           verbose output of operations",
> +	"-s|--subvolid          subvolume id of the to be removed subvolume",
>   	NULL
>   };
>   
> @@ -246,11 +248,12 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd,
>   	char	*dname, *vname, *cpath;
>   	char	*dupdname = NULL;
>   	char	*dupvname = NULL;
> -	char	*path;
> +	char	*path = NULL;
>   	DIR	*dirstream = NULL;
>   	int verbose = 0;
>   	int commit_mode = 0;
>   	u8 fsid[BTRFS_FSID_SIZE];
> +	u64 objectid = 0;
>   	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
>   	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = { NULL, };
>   	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
> @@ -262,11 +265,12 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd,
>   		static const struct option long_options[] = {
>   			{"commit-after", no_argument, NULL, 'c'},
>   			{"commit-each", no_argument, NULL, 'C'},
> +			{"subvolid", required_argument, NULL, 's'},
>   			{"verbose", no_argument, NULL, 'v'},
>   			{NULL, 0, NULL, 0}
>   		};
>   
> -		c = getopt_long(argc, argv, "cCv", long_options, NULL);
> +		c = getopt_long(argc, argv, "cCvs:", long_options, NULL);
>   		if (c < 0)
>   			break;
>   
> @@ -280,6 +284,9 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd,
>   		case 'v':
>   			verbose++;
>   			break;
> +		case 's':
> +			objectid = arg_strtou64(optarg);
> +			break;
>   		default:
>   			usage_unknown_option(cmd, argv);
>   		}
> @@ -288,6 +295,10 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd,
>   	if (check_argc_min(argc - optind, 1))
>   		return 1;
>   
> +	/* when using --subvolid, ensure that we have only one argument */
> +	if (objectid > 0 && check_argc_exact(argc - optind, 1))
> +		return 1;
> +
>   	if (verbose > 0) {
>   		printf("Transaction commit: %s\n",
>   			!commit_mode ? "none (default)" :
> @@ -296,8 +307,30 @@ static int cmd_subvol_delete(const struct cmd_struct *cmd,
>   
>   	cnt = optind;
>   
> +	/* check the following syntax: subvolume delete --subvolid <volid> <path> */
                                                                    ^sub

Thanks, Anand


> +	if (objectid > 0) {
> +		char *subvol, full_volpath[BTRFS_SUBVOL_NAME_MAX];
> +
> +		path = argv[cnt];
> +		err = btrfs_util_subvolume_path(path, objectid, &subvol);
> +		if (err) {
> +			error_btrfs_util(err);
> +			ret = 1;
> +			goto out;
> +		}
> +
> +		/* build new volpath using the volume name found */
> +		sprintf(full_volpath, "%s/%s", path, subvol);
> +		free(subvol);
> +
> +		/* update path to the built path from the subvol id */
> +		path = full_volpath;
> +	}
> +
>   again:
> -	path = argv[cnt];
> +	/* if subvolid is used, path will already be populated */
> +	if (objectid == 0)
> +		path = argv[cnt];
>   
>   	err = btrfs_util_is_subvolume(path);
>   	if (err) {
> diff --git a/tests/misc-tests/038-delete-subvolume/test.sh b/tests/misc-tests/038-delete-subvolume/test.sh
> new file mode 100755
> index 00000000..9137e0d3
> --- /dev/null
> +++ b/tests/misc-tests/038-delete-subvolume/test.sh
> @@ -0,0 +1,30 @@
> +#!/bin/bash
> +# test btrfs subvolume delete --subvolid <volid> <path>
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq mkfs.btrfs
> +check_prereq btrfs
> +
> +setup_root_helper
> +prepare_test_dev
> +
> +run_check_mkfs_test_dev
> +run_check_mount_test_dev
> +
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT"/mysubvol1
> +
> +# subvolid expected failures
> +run_mustfail "subvolume delete --subvolid expects an integer" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid aaa "$TEST_MNT"
> +
> +run_mustfail "subvolume delete --subvolid with invalid unexisting subvolume" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 999 "$TEST_MNT"
> +
> +run_mustfail "subvolume delete --subvolid expects only one extra argument: the mountpoint" \
> +	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 256 "$TEST_MNT" "$TEST_MNT"
> +
> +# delete the recently created subvol using the subvolid
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 256 "$TEST_MNT"
> +
> +run_check_umount_test_dev
>
David Sterba Oct. 29, 2019, 1:51 p.m. UTC | #2
On Tue, Oct 29, 2019 at 10:05:25AM +0800, Anand Jain wrote:
> > @@ -223,6 +223,7 @@ static int wait_for_commit(int fd)
> >   
> >   static const char * const cmd_subvol_delete_usage[] = {
> >   	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
> > +	"btrfs subvolume delete [options] [--subvolid <subvolid> <path>]",
> 
> 
> It should rather be..
> 
> + "btrfs subvolume delete [options] <[-s|--subvolid <subvolid> <path>] | 
> [<subvolume>...]>"

This is hard to understand on the first read, so the preferred way is to
split into more lines when an option changes the overall behaviour, eg.
what plain receive and receive --dump does:


$ ./btrfs receive --help
usage: btrfs receive [options] <mount>
       btrfs receive --dump [options]
Anand Jain Oct. 29, 2019, 6:25 p.m. UTC | #3
On 29/10/19 9:51 PM, David Sterba wrote:
> On Tue, Oct 29, 2019 at 10:05:25AM +0800, Anand Jain wrote:
>>> @@ -223,6 +223,7 @@ static int wait_for_commit(int fd)
>>>    
>>>    static const char * const cmd_subvol_delete_usage[] = {
>>>    	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
>>> +	"btrfs subvolume delete [options] [--subvolid <subvolid> <path>]",
>>
>>
>> It should rather be..
>>
>> + "btrfs subvolume delete [options] <[-s|--subvolid <subvolid> <path>] |
>> [<subvolume>...]>"
> 
> This is hard to understand on the first read, so the preferred way is to
> split into more lines when an option changes the overall behaviour, eg.
> what plain receive and receive --dump does:
> 
> 
> $ ./btrfs receive --help
> usage: btrfs receive [options] <mount>
>         btrfs receive --dump [options]
> 

yep makes sense.

Patch
diff mbox series

diff --git a/Documentation/btrfs-subvolume.asciidoc b/Documentation/btrfs-subvolume.asciidoc
index 49c72e89..9492fc19 100644
--- a/Documentation/btrfs-subvolume.asciidoc
+++ b/Documentation/btrfs-subvolume.asciidoc
@@ -59,12 +59,17 @@  directory.
 Add the newly created subvolume to a qgroup. This option can be given multiple
 times.
 
-*delete* [options] <subvolume> [<subvolume>...]::
+*delete* [options] <subvolume> [<subvolume>...]
+
+*delete* [options] [--subvolid <subvolid> <path>]::
 Delete the subvolume(s) from the filesystem.
 +
 If <subvolume> is not a subvolume, btrfs returns an error but continues if
 there are more arguments to process.
 +
+If --subvolid if used, <path> must point to a btrfs filesystem. See `btrfs
+subvolume list` how to get the subvolume id.
++
 The corresponding directory is removed instantly but the data blocks are
 removed later in the background. The command returns immediately. See `btrfs
 subvolume sync` how to wait until the subvolume gets completely removed.
@@ -84,6 +89,9 @@  wait for transaction commit after deleting each subvolume.
 +
 -v|--verbose::::
 verbose output of operations.
++
+-s|--subvolid::::
+subvolume id of the to be removed subvolume from <path>
 
 *find-new* <subvolume> <last_gen>::
 List the recently modified files in a subvolume, after <last_gen> generation.
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 7a5fd79b..df75f82d 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -223,6 +223,7 @@  static int wait_for_commit(int fd)
 
 static const char * const cmd_subvol_delete_usage[] = {
 	"btrfs subvolume delete [options] <subvolume> [<subvolume>...]",
+	"btrfs subvolume delete [options] [--subvolid <subvolid> <path>]",
 	"Delete subvolume(s)",
 	"Delete subvolumes from the filesystem. The corresponding directory",
 	"is removed instantly but the data blocks are removed later.",
@@ -234,6 +235,7 @@  static const char * const cmd_subvol_delete_usage[] = {
 	"-c|--commit-after      wait for transaction commit at the end of the operation",
 	"-C|--commit-each       wait for transaction commit after deleting each subvolume",
 	"-v|--verbose           verbose output of operations",
+	"-s|--subvolid          subvolume id of the to be removed subvolume",
 	NULL
 };
 
@@ -246,11 +248,12 @@  static int cmd_subvol_delete(const struct cmd_struct *cmd,
 	char	*dname, *vname, *cpath;
 	char	*dupdname = NULL;
 	char	*dupvname = NULL;
-	char	*path;
+	char	*path = NULL;
 	DIR	*dirstream = NULL;
 	int verbose = 0;
 	int commit_mode = 0;
 	u8 fsid[BTRFS_FSID_SIZE];
+	u64 objectid = 0;
 	char uuidbuf[BTRFS_UUID_UNPARSED_SIZE];
 	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = { NULL, };
 	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
@@ -262,11 +265,12 @@  static int cmd_subvol_delete(const struct cmd_struct *cmd,
 		static const struct option long_options[] = {
 			{"commit-after", no_argument, NULL, 'c'},
 			{"commit-each", no_argument, NULL, 'C'},
+			{"subvolid", required_argument, NULL, 's'},
 			{"verbose", no_argument, NULL, 'v'},
 			{NULL, 0, NULL, 0}
 		};
 
-		c = getopt_long(argc, argv, "cCv", long_options, NULL);
+		c = getopt_long(argc, argv, "cCvs:", long_options, NULL);
 		if (c < 0)
 			break;
 
@@ -280,6 +284,9 @@  static int cmd_subvol_delete(const struct cmd_struct *cmd,
 		case 'v':
 			verbose++;
 			break;
+		case 's':
+			objectid = arg_strtou64(optarg);
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -288,6 +295,10 @@  static int cmd_subvol_delete(const struct cmd_struct *cmd,
 	if (check_argc_min(argc - optind, 1))
 		return 1;
 
+	/* when using --subvolid, ensure that we have only one argument */
+	if (objectid > 0 && check_argc_exact(argc - optind, 1))
+		return 1;
+
 	if (verbose > 0) {
 		printf("Transaction commit: %s\n",
 			!commit_mode ? "none (default)" :
@@ -296,8 +307,30 @@  static int cmd_subvol_delete(const struct cmd_struct *cmd,
 
 	cnt = optind;
 
+	/* check the following syntax: subvolume delete --subvolid <volid> <path> */
+	if (objectid > 0) {
+		char *subvol, full_volpath[BTRFS_SUBVOL_NAME_MAX];
+
+		path = argv[cnt];
+		err = btrfs_util_subvolume_path(path, objectid, &subvol);
+		if (err) {
+			error_btrfs_util(err);
+			ret = 1;
+			goto out;
+		}
+
+		/* build new volpath using the volume name found */
+		sprintf(full_volpath, "%s/%s", path, subvol);
+		free(subvol);
+
+		/* update path to the built path from the subvol id */
+		path = full_volpath;
+	}
+
 again:
-	path = argv[cnt];
+	/* if subvolid is used, path will already be populated */
+	if (objectid == 0)
+		path = argv[cnt];
 
 	err = btrfs_util_is_subvolume(path);
 	if (err) {
diff --git a/tests/misc-tests/038-delete-subvolume/test.sh b/tests/misc-tests/038-delete-subvolume/test.sh
new file mode 100755
index 00000000..9137e0d3
--- /dev/null
+++ b/tests/misc-tests/038-delete-subvolume/test.sh
@@ -0,0 +1,30 @@ 
+#!/bin/bash
+# test btrfs subvolume delete --subvolid <volid> <path>
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+prepare_test_dev
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT"/mysubvol1
+
+# subvolid expected failures
+run_mustfail "subvolume delete --subvolid expects an integer" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid aaa "$TEST_MNT"
+
+run_mustfail "subvolume delete --subvolid with invalid unexisting subvolume" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 999 "$TEST_MNT"
+
+run_mustfail "subvolume delete --subvolid expects only one extra argument: the mountpoint" \
+	$SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 256 "$TEST_MNT" "$TEST_MNT"
+
+# delete the recently created subvol using the subvolid
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --subvolid 256 "$TEST_MNT"
+
+run_check_umount_test_dev