mkfs --subvol fixups

Documentation/mkfs.btrfs.rst

@@ -155,15 +155,26 @@ OPTIONS
                 contain the files from *rootdir*. Since version 4.14.1 the filesystem size is
                 not minimized. Please see option *--shrink* if you need that functionality.
 
--u|--subvol <subdir>:<flags>
+-u|--subvol <type>:<subdir>
         Specify that *subdir* is to be created as a subvolume rather than a regular
         directory.  The option *--rootdir* must also be specified, and *subdir* must be an
         existing subdirectory within it.  This option can be specified multiple times.
 
-        *flags* is an optional comma-separated list of modifiers. Valid choices are:
+        *type* is an optional additional modifier. Valid choices are:
 
-        * *default*: create as default subvolume (this can only be specified once)
-        * *ro*: create as readonly subvolume
+        * *default*: create as default subvolume
+        * *ro*: create as read-only subvolume
+        * *rw*: create as read-write subvolume (the default)
+        * *default-ro*: create as read-only default subvolume
+
+        Only one of *default* and *default-ro* may be specified.
+
+        If you wish to create a subvolume with a name containing a colon and you don't
+        want this to be parsed as containing a modifier, you can prefix the path with `./`:
+
+        .. code-block:: bash
+
+                $ mkfs.btrfs --rootdir dir --subvol ./ro:subdir /dev/loop0
 
 	If there are hard links inside *rootdir* and *subdir* will split the
 	subvolumes, like the following case::

mkfs/main.c

@@ -440,7 +440,7 @@ static const char * const mkfs_usage[] = {
 	"Creation:",
 	OPTLINE("-b|--byte-count SIZE", "set size of each device to SIZE (filesystem size is sum of all device sizes)"),
 	OPTLINE("-r|--rootdir DIR", "copy files from DIR to the image root directory"),
-	OPTLINE("-u|--subvol SUBDIR:FLAGS", "create SUBDIR as subvolume rather than normal directory, can be specified multiple times"),
+	OPTLINE("-u|--subvol TYPE:SUBDIR", "create SUBDIR as subvolume rather than normal directory, can be specified multiple times"),
 	OPTLINE("--shrink", "(with --rootdir) shrink the filled filesystem to minimal size"),
 	OPTLINE("-K|--nodiscard", "do not perform whole device TRIM"),
 	OPTLINE("-f|--force", "force overwrite of existing filesystem"),
@@ -1015,48 +1015,6 @@ static void *prepare_one_device(void *ctx)
 	return NULL;
 }
 
-static int parse_subvol_flags(struct rootdir_subvol *subvol, const char *flags)
-{
-	char *buf, *orig_buf;
-	int ret;
-
-	buf = orig_buf = strdup(flags);
-
-	if (!buf) {
-		error_msg(ERROR_MSG_MEMORY, NULL);
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	while (true) {
-		char *comma = strstr(buf, ",");
-
-		if (comma)
-			*comma = 0;
-
-		if (!strcmp(buf, "default")) {
-			subvol->is_default = true;
-		} else if (!strcmp(buf, "ro")) {
-			subvol->readonly = true;
-		} else if (buf[0] != 0) {
-			error("unrecognized subvol flag \"%s\"", buf);
-			ret = 1;
-			goto out;
-		}
-
-		if (comma)
-			buf = comma + 1;
-		else
-			break;
-	}
-
-	ret = 0;
-
-out:
-	free(orig_buf);
-	return ret;
-}
-
 int BOX_MAIN(mkfs)(int argc, char **argv)
 {
 	char *file;
@@ -1098,7 +1056,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 	char *label = NULL;
 	int nr_global_roots = sysconf(_SC_NPROCESSORS_ONLN);
 	char *source_dir = NULL;
-	size_t source_dir_len = 0;
 	struct rootdir_subvol *rds;
 	bool has_default_subvol = false;
 	LIST_HEAD(subvols);
@@ -1259,6 +1216,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 			case 'u': {
 				struct rootdir_subvol *subvol;
 				char *colon;
+				bool valid_prefix = false;
 
 				subvol = calloc(1, sizeof(struct rootdir_subvol));
 				if (!subvol) {
@@ -1270,32 +1228,32 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 				colon = strstr(optarg, ":");
 
 				if (colon) {
-					/* Make sure we choose the last colon in
-					 * optarg, in case the subvol name
-					 * itself contains a colon.  */
-					do {
-						char *colon2;
-
-						colon2 = strstr(colon + 1, ":");
-
-						if (colon2)
-							colon = colon2;
-						else
-							break;
-					} while (true);
-
-					subvol->dir = strndup(optarg, colon - optarg);
-					if (parse_subvol_flags(subvol, colon + 1)) {
-						ret = 1;
-						goto error;
+					if (!string_has_prefix(optarg, "default:")) {
+						subvol->is_default = true;
+						valid_prefix = true;
+					} else if (!string_has_prefix(optarg, "ro:")) {
+						subvol->readonly = true;
+						valid_prefix = true;
+					} else if (!string_has_prefix(optarg, "rw:")) {
+						subvol->readonly = false;
+						valid_prefix = true;
+					} else if (!string_has_prefix(optarg, "default-ro:")) {
+						subvol->is_default = true;
+						subvol->readonly = true;
+						valid_prefix = true;
 					}
-				} else {
-					subvol->dir = strdup(optarg);
+				}
+
+				if (arg_copy_path(subvol->dir, valid_prefix ? colon + 1 : optarg,
+						  sizeof(subvol->dir))) {
+					error("--subvol path too long");
+					ret = 1;
+					goto error;
 				}
 
 				if (subvol->is_default) {
 					if (has_default_subvol) {
-						error("subvol default flag can only be specified once");
+						error("default subvol can only be specified once");
 						ret = 1;
 						goto error;
 					}
@@ -1386,57 +1344,37 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 
 		free(source_dir);
 		source_dir = canonical;
-		source_dir_len = strlen(source_dir);
 	}
 
 	list_for_each_entry(rds, &subvols, list) {
-		char *path, *canonical;
+		char path[PATH_MAX];
 		struct rootdir_subvol *rds2;
-		size_t dir_len;
-
-		dir_len = strlen(rds->dir);
 
-		path = malloc(source_dir_len + 1 + dir_len + 1);
-		if (!path) {
-			error_msg(ERROR_MSG_MEMORY, NULL);
+		if (path_cat_out(path, source_dir, rds->dir)) {
+			error("path invalid");
 			ret = 1;
 			goto error;
 		}
 
-		memcpy(path, source_dir, source_dir_len);
-		path[source_dir_len] = '/';
-		memcpy(path + source_dir_len + 1, rds->dir, dir_len + 1);
-
-		canonical = realpath(path, NULL);
-		if (!canonical) {
+		if (!realpath(path, rds->full_path)) {
 			error("could not get canonical path to %s", rds->dir);
-			free(path);
 			ret = 1;
 			goto error;
 		}
 
-		free(path);
-		path = canonical;
-
-		if (!path_exists(path)) {
+		if (!path_exists(rds->full_path)) {
 			error("subvolume %s does not exist", rds->dir);
-			free(path);
 			ret = 1;
 			goto error;
 		}
 
-		if (!path_is_dir(path)) {
+		if (!path_is_dir(rds->full_path)) {
 			error("subvolume %s is not a directory", rds->dir);
-			free(path);
 			ret = 1;
 			goto error;
 		}
 
-		rds->full_path = path;
-
-		if (strlen(path) < source_dir_len + 1 ||
-		    memcmp(path, source_dir, source_dir_len) != 0 ||
-		    path[source_dir_len] != '/') {
+		if (!path_is_in_dir(source_dir, rds->full_path)) {
 			error("subvolume %s is not a child of %s", rds->dir, source_dir);
 			ret = 1;
 			goto error;
@@ -1445,7 +1383,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 		for (rds2 = list_first_entry(&subvols, struct rootdir_subvol, list);
 		     rds2 != rds;
 		     rds2 = list_next_entry(rds2, list)) {
-			if (strcmp(rds2->full_path, path) == 0) {
+			if (strcmp(rds2->full_path, rds->full_path) == 0) {
 				error("subvolume %s specified more than once", rds->dir);
 				ret = 1;
 				goto error;
@@ -2156,8 +2094,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 		struct rootdir_subvol *head;
 
 		head = list_entry(subvols.next, struct rootdir_subvol, list);
-		free(head->dir);
-		free(head->full_path);
 		list_del(&head->list);
 		free(head);
 	}

mkfs/rootdir.c

@@ -711,9 +711,6 @@ static int ftw_add_inode(const char *full_path, const struct stat *st,
 				ret = ftw_add_subvol(full_path, st, typeflag,
 						     ftwbuf, rds);
 
-				free(rds->dir);
-				free(rds->full_path);
-
 				list_del(&rds->list);
 				free(rds);
 

mkfs/rootdir.h

@@ -30,8 +30,8 @@ struct btrfs_root;
 
 struct rootdir_subvol {
 	struct list_head list;
-	char *dir;
-	char *full_path;
+	char dir[PATH_MAX];
+	char full_path[PATH_MAX];
 	bool is_default;
 	bool readonly;
 };

tests/mkfs-tests/036-rootdir-subvol/test.sh

@@ -18,19 +18,53 @@ basic()
 	run_check mkdir "$tmp/dir/subvol"
 	run_check touch "$tmp/dir/subvol/bar"
 
-	run_check_mkfs_test_dev --rootdir "$tmp" --subvol dir/subvol
+	if [ "$1" != "" ]; then
+		run_check_mkfs_test_dev --rootdir "$tmp" --subvol $1:dir/subvol
+	else
+		run_check_mkfs_test_dev --rootdir "$tmp" --subvol dir/subvol
+	fi
+
 	run_check $SUDO_HELPER "$TOP/btrfs" check "$TEST_DEV"
 
-	run_check_mount_test_dev
+	run_check_mount_test_dev -o subvolid=5
 	run_check_stdout $SUDO_HELPER "$TOP/btrfs" subvolume list "$TEST_MNT" | \
 	cut -d\  -f9 > "$tmp/output"
+	run_check_stdout "$TOP/btrfs" property get "$TEST_MNT/dir/subvol" ro | \
+	cut -d = -f2 > "$tmp/output2"
+	run_check_stdout "$TOP/btrfs" subvolume get-default "$TEST_MNT" | \
+	cut -d\  -f2 > "$tmp/output3"
 	run_check_umount_test_dev
 
 	result=$(cat "$tmp/output")
 
 	if [ "$result" != "dir/subvol" ]; then
 		_fail "dir/subvol not in subvolume list"
 	fi
+
+	result=$(cat "$tmp/output2")
+
+	if [ "$1" == "ro" -o "$1" == "default-ro" ]; then
+		if [ "$result" != "true" ]; then
+			 _fail "dir/subvol was read-write, expected read-only"
+		fi
+	else
+		if [ "$result" != "false" ]; then
+			_fail "dir/subvol was read-only, expected read-write"
+		fi
+	fi
+
+	result=$(cat "$tmp/output3")
+
+	if [ "$1" == "default" -o "$1" == "default-ro" ]; then
+		if [ "$result" != "256" ]; then
+			 _fail "default subvol was $result, expected 256"
+		fi
+	else
+		if [ "$result" != "5" ]; then
+			_fail "default subvol was $result, expected 5"
+		fi
+	fi
+
 	rm -rf -- "$tmp/foo" "$tmp/dir"
 }
 
@@ -61,10 +95,15 @@ split_by_subvolume_hardlinks()
 	run_check mkdir "$tmp/subv"
 	run_check ln "$tmp/hl1" "$tmp/subv/hl3"
 
-	run_check_mkfs_test_dev --rootdir "$tmp" --subvol subv
+	if [ "$1" != "" ]; then
+		run_check_mkfs_test_dev --rootdir "$tmp" --subvol $1:subv
+	else
+		run_check_mkfs_test_dev --rootdir "$tmp" --subvol subv
+	fi
+
 	run_check $SUDO_HELPER "$TOP/btrfs" check "$TEST_DEV"
 
-	run_check_mount_test_dev
+	run_check_mount_test_dev -o subvolid=5
 	nr_hardlink=$(run_check_stdout $SUDO_HELPER stat -c "%h" "$TEST_MNT/hl1")
 
 	if [ "$nr_hardlink" -ne 2 ]; then
@@ -76,10 +115,14 @@ split_by_subvolume_hardlinks()
 		_fail "hard link number incorrect for subv/hl3, has ${nr_hardlink} expect 1"
 	fi
 	run_check_umount_test_dev
-	rm -rf -- "$tmp/hl1" "$tmp/hl2" "$tmp/dir"
+	rm -rf -- "$tmp/hl1" "$tmp/hl2" "$tmp/subv"
 }
 
-basic
+for mod in "" ro rw default default-ro;
+do
+	basic $mod
+	split_by_subvolume_hardlinks $mod
+done
+
 basic_hardlinks
-split_by_subvolume_hardlinks
 rm -rf -- "$tmp"