[PATCH 1/3] btrfs-progs: check option conflict for btrfs-convert

[PATCH 1/3] btrfs-progs: check option conflict for btrfs-convert

[Gui Hecheng]

The -d, -i, -n options make no sense to rollback.
Check the improper usages such as:
	# btrfs-convert -r -d <dev>

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-convert.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 952e3e6..2ea6a09 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2699,6 +2699,7 @@ int main(int argc, char *argv[])
 	int noxattr = 0;
 	int datacsum = 1;
 	int rollback = 0;
+	int usage_error = 0;
 	char *file;
 	while(1) {
 		int c = getopt(argc, argv, "dinr");
@@ -2729,6 +2730,18 @@ int main(int argc, char *argv[])
 		return 1;
 	}
 
+	if (rollback) {
+		if (!datacsum || noxattr || !packing) {
+			fprintf(stderr, "Usage error: -d, -i, -n options do not apply to rollback\n");
+			usage_error++;
+		}
+	}
+
+	if (usage_error) {
+		print_usage();
+		return 1;
+	}
+
 	file = argv[optind];
 	ret = check_mounted(file);
 	if (ret < 0) {
-- 
1.8.1.4

[PATCH 2/3] btrfs-progs: remove unused parameter in rollback for btrfs-convert

[Gui Hecheng]

The @force parameter for function @do_rollback is never checked
or used, remove it.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-convert.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 2ea6a09..21bbc70 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2376,7 +2376,7 @@ fail:
 	return -1;
 }
 
-static int do_rollback(const char *devname, int force)
+static int do_rollback(const char *devname)
 {
 	int fd = -1;
 	int ret;
@@ -2754,7 +2754,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (rollback) {
-		ret = do_rollback(file, 0);
+		ret = do_rollback(file);
 	} else {
 		ret = do_convert(file, datacsum, packing, noxattr);
 	}
-- 
1.8.1.4

[PATCH 3/3] btrfs-progs: make close_ctree return void

[Gui Hecheng]

The close_ctree always returns 0 and the stuff that depends on
its return value is of no sense.
Just make close_ctree return void.

Signed-off-by: Gui Hecheng <guihc.fnst@cn.fujitsu.com>
---
 btrfs-convert.c    | 14 +++-----------
 btrfs-debug-tree.c |  3 ++-
 btrfs-image.c      |  4 ++--
 disk-io.c          |  3 +--
 disk-io.h          |  2 +-
 mkfs.c             |  3 +--
 6 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 21bbc70..c685fcf 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2297,11 +2297,7 @@ static int do_convert(const char *devname, int datacsum, int packing,
 		fprintf(stderr, "error during cleanup_sys_chunk %d\n", ret);
 		goto fail;
 	}
-	ret = close_ctree(root);
-	if (ret) {
-		fprintf(stderr, "error during close_ctree %d\n", ret);
-		goto fail;
-	}
+	close_ctree(root);
 	close_ext2fs(ext2_fs);
 
 	/*
@@ -2325,7 +2321,7 @@ static int do_convert(const char *devname, int datacsum, int packing,
 		fprintf(stderr, "error during fixup_chunk_tree\n");
 		goto fail;
 	}
-	ret = close_ctree(root);
+	close_ctree(root);
 	close(fd);
 
 	printf("conversion complete.\n");
@@ -2591,11 +2587,7 @@ next_extent:
 	ret = btrfs_commit_transaction(trans, root);
 	BUG_ON(ret);
 
-	ret = close_ctree(root);
-	if (ret) {
-		fprintf(stderr, "error during close_ctree %d\n", ret);
-		goto fail;
-	}
+	close_ctree(root);
 
 	/* zero btrfs super block mirrors */
 	memset(buf, 0, sectorsize);
diff --git a/btrfs-debug-tree.c b/btrfs-debug-tree.c
index e46500d..aeeeb74 100644
--- a/btrfs-debug-tree.c
+++ b/btrfs-debug-tree.c
@@ -408,5 +408,6 @@ no_node:
 	printf("uuid %s\n", uuidbuf);
 	printf("%s\n", BTRFS_BUILD_VERSION);
 close_root:
-	return close_ctree(root);
+	close_ctree(root);
+	return 0;
 }
diff --git a/btrfs-image.c b/btrfs-image.c
index 985aa26..bb3d0a9 100644
--- a/btrfs-image.c
+++ b/btrfs-image.c
@@ -1333,8 +1333,8 @@ out:
 	metadump_destroy(&metadump, num_threads);
 
 	btrfs_free_path(path);
-	ret = close_ctree(root);
-	return err ? err : ret;
+	close_ctree(root);
+	return err ? err : 0;
 }
 
 static void update_super_old(u8 *buffer)
diff --git a/disk-io.c b/disk-io.c
index d10d647..f13d46d 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1410,7 +1410,7 @@ int write_ctree_super(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int close_ctree(struct btrfs_root *root)
+void close_ctree(struct btrfs_root *root)
 {
 	int ret;
 	struct btrfs_trans_handle *trans;
@@ -1436,7 +1436,6 @@ int close_ctree(struct btrfs_root *root)
 	btrfs_close_devices(fs_info->fs_devices);
 	btrfs_cleanup_all_caches(fs_info);
 	btrfs_free_fs_info(fs_info);
-	return 0;
 }
 
 int clean_tree_block(struct btrfs_trans_handle *trans, struct btrfs_root *root,
diff --git a/disk-io.h b/disk-io.h
index 13d4420..87a69b5 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -78,7 +78,7 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 struct btrfs_fs_info *open_ctree_fs_info(const char *filename,
 					 u64 sb_bytenr, u64 root_tree_bytenr,
 					 enum btrfs_open_ctree_flags flags);
-int close_ctree(struct btrfs_root *root);
+void close_ctree(struct btrfs_root *root);
 int write_all_supers(struct btrfs_root *root);
 int write_ctree_super(struct btrfs_trans_handle *trans,
 		      struct btrfs_root *root);
diff --git a/mkfs.c b/mkfs.c
index d980d4a..5e55e72 100644
--- a/mkfs.c
+++ b/mkfs.c
@@ -1661,8 +1661,7 @@ raid_groups:
 		BUG_ON(ret);
 	}
 
-	ret = close_ctree(root);
-	BUG_ON(ret);
+	close_ctree(root);
 	free(label);
 	return 0;
 }
-- 
1.8.1.4

Re: [PATCH 3/3] btrfs-progs: make close_ctree return void

[David Sterba]

On Thu, Aug 07, 2014 at 10:35:59AM +0800, Gui Hecheng wrote:
> The close_ctree always returns 0 and the stuff that depends on
> its return value is of no sense.
> Just make close_ctree return void.

You should not do that if the function contains BUG_ONs, this means the
error path is not handled, rather than trivial.