[PATCH 0/3] add_qgroup/update_qgroup/__qgroup_search cleanup

[PATCH 0/3] add_qgroup/update_qgroup/__qgroup_search cleanup

[Lu Fengqi]

Patch 1 just remove the redundant function add_qgroup.
Patch 2 separate QGROUP_INFO/QGROUP_LIMIT/QGROUP_RELATION process from
the update_qgroup.
Patch 3 mainly the coding style adjustment.

Lu Fengqi (3):
  btrfs-progs: qgroup: cleanup the redundant function add_qgroup
  btrfs-progs: qgroup: split update_qgroup to reduce arguments
  btrfs-progs: qgroup: cleanup __qgroup_search, no functional change

 cmds-qgroup.c |   4 -
 qgroup.c      | 374 ++++++++++++++++++++++++++++------------------------------
 2 files changed, 180 insertions(+), 198 deletions(-)

-- 
2.14.3

[PATCH 1/3] btrfs-progs: qgroup: cleanup the redundant function add_qgroup

[Lu Fengqi]

There are reusable parts between update_qgroup and add_qgroup. So
introduce the function get_or_add_qgroup and use update_qgroup instead
of add_qgroup.
No functional changes.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 qgroup.c | 119 ++++++++++++++++++++++++++-------------------------------------
 1 file changed, 48 insertions(+), 71 deletions(-)

diff --git a/qgroup.c b/qgroup.c
index 7e8ef079..032e6cad 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -578,6 +578,47 @@ static struct btrfs_qgroup *qgroup_tree_search(struct qgroup_lookup *root_tree,
 	return NULL;
 }
 
+/*
+ * Lookup or insert btrfs_qgroup in the qgroup_lookup.
+ *
+ * Search a btrfs_qgroup with @qgroupid from the @qgroup_lookup. If not found,
+ * initialize a btrfs_qgroup with the given qgroupid and insert it to the
+ * @qgroup_lookup.
+ *
+ * Return the pointer to the btrfs_qgroup if found or insert successfully.
+ * Return NULL if any error occurred.
+ */
+static struct btrfs_qgroup *get_or_add_qgroup(
+		struct qgroup_lookup *qgroup_lookup, u64 qgroupid)
+{
+	struct btrfs_qgroup *bq;
+	int ret;
+
+	bq = qgroup_tree_search(qgroup_lookup, qgroupid);
+	if (bq)
+		return bq;
+
+	bq = calloc(1, sizeof(*bq));
+	if (!bq) {
+		error("memory allocation failed");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	bq->qgroupid = qgroupid;
+	INIT_LIST_HEAD(&bq->qgroups);
+	INIT_LIST_HEAD(&bq->members);
+
+	ret = qgroup_tree_insert(qgroup_lookup, bq);
+	if (ret) {
+		error("failed to insert %llu into tree: %s",
+		       (unsigned long long)bq->qgroupid, strerror(-ret));
+		free(bq);
+		return ERR_PTR(ret);
+	}
+
+	return bq;
+}
+
 static int update_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
 			 u64 generation, u64 rfer, u64 rfer_cmpr, u64 excl,
 			 u64 excl_cmpr, u64 flags, u64 max_rfer, u64 max_excl,
@@ -587,9 +628,9 @@ static int update_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
 	struct btrfs_qgroup *bq;
 	struct btrfs_qgroup_list *list;
 
-	bq = qgroup_tree_search(qgroup_lookup, qgroupid);
-	if (!bq || bq->qgroupid != qgroupid)
-		return -ENOENT;
+	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
+	if (IS_ERR_OR_NULL(bq))
+		return PTR_ERR(bq);
 
 	if (generation)
 		bq->generation = generation;
@@ -613,7 +654,7 @@ static int update_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
 		list = malloc(sizeof(*list));
 		if (!list) {
 			error("memory allocation failed");
-			exit(1);
+			return -ENOMEM;
 		}
 		list->qgroup = pa;
 		list->member = child;
@@ -623,70 +664,6 @@ static int update_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
 	return 0;
 }
 
-static int add_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
-		      u64 generation, u64 rfer, u64 rfer_cmpr, u64 excl,
-		      u64 excl_cmpr, u64 flags, u64 max_rfer, u64 max_excl,
-		      u64 rsv_rfer, u64 rsv_excl, struct btrfs_qgroup *parent,
-		      struct btrfs_qgroup *child)
-{
-	struct btrfs_qgroup *bq;
-	struct btrfs_qgroup_list *list;
-	int ret;
-
-	ret = update_qgroup(qgroup_lookup, qgroupid, generation, rfer,
-			    rfer_cmpr, excl, excl_cmpr, flags, max_rfer,
-			    max_excl, rsv_rfer, rsv_excl, parent, child);
-	if (!ret)
-		return 0;
-
-	bq = calloc(1, sizeof(*bq));
-	if (!bq) {
-		error("memory allocation failed");
-		exit(1);
-	}
-	if (qgroupid) {
-		bq->qgroupid = qgroupid;
-		INIT_LIST_HEAD(&bq->qgroups);
-		INIT_LIST_HEAD(&bq->members);
-	}
-	if (generation)
-		bq->generation = generation;
-	if (rfer)
-		bq->rfer = rfer;
-	if (rfer_cmpr)
-		bq->rfer_cmpr = rfer_cmpr;
-	if (excl)
-		bq->excl = excl;
-	if (excl_cmpr)
-		bq->excl_cmpr = excl_cmpr;
-	if (flags)
-		bq->flags = flags;
-	if (max_rfer)
-		bq->max_rfer = max_rfer;
-	if (max_excl)
-		bq->max_excl = max_excl;
-	if (rsv_rfer)
-		bq->rsv_rfer = rsv_rfer;
-	if (parent && child) {
-		list = malloc(sizeof(*list));
-		if (!list) {
-			error("memory allocation failed");
-			exit(1);
-		}
-		list->qgroup = parent;
-		list->member = child;
-		list_add_tail(&list->next_qgroup, &child->qgroups);
-		list_add_tail(&list->next_member, &parent->members);
-	}
-	ret = qgroup_tree_insert(qgroup_lookup, bq);
-	if (ret) {
-		error("failed to insert %llu into tree: %s",
-		       (unsigned long long)bq->qgroupid, strerror(-ret));
-		exit(1);
-	}
-	return ret;
-}
-
 static void __free_btrfs_qgroup(struct btrfs_qgroup *bq)
 {
 	struct btrfs_qgroup_list *list;
@@ -1103,7 +1080,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 				a5 =
 				  btrfs_stack_qgroup_info_exclusive_compressed
 				  (info);
-				add_qgroup(qgroup_lookup,
+				update_qgroup(qgroup_lookup,
 					btrfs_search_header_offset(sh), a1,
 					a2, a3, a4, a5, 0, 0, 0, 0, 0, NULL,
 					NULL);
@@ -1121,7 +1098,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 				     (limit);
 				a5 = btrfs_stack_qgroup_limit_rsv_exclusive
 				     (limit);
-				add_qgroup(qgroup_lookup,
+				update_qgroup(qgroup_lookup,
 					   btrfs_search_header_offset(sh), 0,
 					   0, 0, 0, 0, a1, a2, a3, a4, a5,
 					   NULL, NULL);
@@ -1138,7 +1115,7 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 					 btrfs_search_header_objectid(sh));
 				if (!bq1)
 					goto skip;
-				add_qgroup(qgroup_lookup,
+				update_qgroup(qgroup_lookup,
 					   btrfs_search_header_offset(sh), 0,
 					   0, 0, 0, 0, 0, 0, 0, 0, 0, bq, bq1);
 			} else
-- 
2.14.3

[PATCH 2/3] btrfs-progs: qgroup: split update_qgroup to reduce arguments

[Lu Fengqi]

The function update_qgroup has too many arguments that are too difficult
to use. Therefore, split it to update_qgroup_info, update_qgroup_limit,
update_qgroup_relation.

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 qgroup.c | 163 +++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 81 insertions(+), 82 deletions(-)

diff --git a/qgroup.c b/qgroup.c
index 032e6cad..10059e71 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -619,48 +619,77 @@ static struct btrfs_qgroup *get_or_add_qgroup(
 	return bq;
 }
 
-static int update_qgroup(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
-			 u64 generation, u64 rfer, u64 rfer_cmpr, u64 excl,
-			 u64 excl_cmpr, u64 flags, u64 max_rfer, u64 max_excl,
-			 u64 rsv_rfer, u64 rsv_excl, struct btrfs_qgroup *pa,
-			 struct btrfs_qgroup *child)
+static int update_qgroup_info(struct qgroup_lookup *qgroup_lookup, u64 qgroupid,
+			      struct btrfs_qgroup_info_item *info)
 {
 	struct btrfs_qgroup *bq;
-	struct btrfs_qgroup_list *list;
 
 	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
 	if (IS_ERR_OR_NULL(bq))
 		return PTR_ERR(bq);
 
-	if (generation)
-		bq->generation = generation;
-	if (rfer)
-		bq->rfer = rfer;
-	if (rfer_cmpr)
-		bq->rfer_cmpr = rfer_cmpr;
-	if (excl)
-		bq->excl = excl;
-	if (excl_cmpr)
-		bq->excl_cmpr = excl_cmpr;
-	if (flags)
-		bq->flags = flags;
-	if (max_rfer)
-		bq->max_rfer = max_rfer;
-	if (max_excl)
-		bq->max_excl = max_excl;
-	if (rsv_rfer)
-		bq->rsv_rfer = rsv_rfer;
-	if (pa && child) {
-		list = malloc(sizeof(*list));
-		if (!list) {
-			error("memory allocation failed");
-			return -ENOMEM;
-		}
-		list->qgroup = pa;
-		list->member = child;
-		list_add_tail(&list->next_qgroup, &child->qgroups);
-		list_add_tail(&list->next_member, &pa->members);
+	bq->generation = btrfs_stack_qgroup_info_generation(info);
+	bq->rfer = btrfs_stack_qgroup_info_referenced(info);
+	bq->rfer_cmpr = btrfs_stack_qgroup_info_referenced_compressed(info);
+	bq->excl = btrfs_stack_qgroup_info_exclusive(info);
+	bq->excl_cmpr = btrfs_stack_qgroup_info_exclusive_compressed(info);
+
+	return 0;
+}
+
+static int update_qgroup_limit(struct qgroup_lookup *qgroup_lookup,
+			       u64 qgroupid,
+			       struct btrfs_qgroup_limit_item *limit)
+{
+	struct btrfs_qgroup *bq;
+
+	bq = get_or_add_qgroup(qgroup_lookup, qgroupid);
+	if (IS_ERR_OR_NULL(bq))
+		return PTR_ERR(bq);
+
+	bq->flags = btrfs_stack_qgroup_limit_flags(limit);
+	bq->max_rfer = btrfs_stack_qgroup_limit_max_referenced(limit);
+	bq->max_excl = btrfs_stack_qgroup_limit_max_exclusive(limit);
+	bq->rsv_rfer = btrfs_stack_qgroup_limit_rsv_referenced(limit);
+	bq->rsv_excl = btrfs_stack_qgroup_limit_rsv_exclusive(limit);
+
+	return 0;
+}
+
+static int update_qgroup_relation(struct qgroup_lookup *qgroup_lookup,
+				  u64 child_id, u64 parent_id)
+{
+	struct btrfs_qgroup *child;
+	struct btrfs_qgroup *parent;
+	struct btrfs_qgroup_list *list;
+
+	child = qgroup_tree_search(qgroup_lookup, child_id);
+	if (!child) {
+		error("cannot find the qgroup %llu/%llu",
+		      btrfs_qgroup_level(child_id),
+		      btrfs_qgroup_subvid(child_id));
+		return -ENOENT;
 	}
+
+	parent = qgroup_tree_search(qgroup_lookup, parent_id);
+	if (!parent) {
+		error("cannot find the qgroup %llu/%llu",
+		      btrfs_qgroup_level(parent_id),
+		      btrfs_qgroup_subvid(parent_id));
+		return -ENOENT;
+	}
+
+	list = malloc(sizeof(*list));
+	if (!list) {
+		error("memory allocation failed");
+		return -ENOMEM;
+	}
+
+	list->qgroup = parent;
+	list->member = child;
+	list_add_tail(&list->next_qgroup, &child->qgroups);
+	list_add_tail(&list->next_member, &parent->members);
+
 	return 0;
 }
 
@@ -1019,13 +1048,8 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 	unsigned int i;
 	struct btrfs_qgroup_info_item *info;
 	struct btrfs_qgroup_limit_item *limit;
-	struct btrfs_qgroup *bq;
-	struct btrfs_qgroup *bq1;
-	u64 a1;
-	u64 a2;
-	u64 a3;
-	u64 a4;
-	u64 a5;
+	u64 qgroupid;
+	u64 qgroupid1;
 
 	memset(&args, 0, sizeof(args));
 
@@ -1069,55 +1093,30 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 				print_status_flag_warning(flags);
 			} else if (btrfs_search_header_type(sh)
 				   == BTRFS_QGROUP_INFO_KEY) {
+				qgroupid = btrfs_search_header_offset(sh);
 				info = (struct btrfs_qgroup_info_item *)
 				       (args.buf + off);
-				a1 = btrfs_stack_qgroup_info_generation(info);
-				a2 = btrfs_stack_qgroup_info_referenced(info);
-				a3 =
-				  btrfs_stack_qgroup_info_referenced_compressed
-				  (info);
-				a4 = btrfs_stack_qgroup_info_exclusive(info);
-				a5 =
-				  btrfs_stack_qgroup_info_exclusive_compressed
-				  (info);
-				update_qgroup(qgroup_lookup,
-					btrfs_search_header_offset(sh), a1,
-					a2, a3, a4, a5, 0, 0, 0, 0, 0, NULL,
-					NULL);
+
+				update_qgroup_info(qgroup_lookup, qgroupid,
+						   info);
 			} else if (btrfs_search_header_type(sh)
 				   == BTRFS_QGROUP_LIMIT_KEY) {
+				qgroupid = btrfs_search_header_offset(sh);
 				limit = (struct btrfs_qgroup_limit_item *)
-				    (args.buf + off);
-
-				a1 = btrfs_stack_qgroup_limit_flags(limit);
-				a2 = btrfs_stack_qgroup_limit_max_referenced
-				     (limit);
-				a3 = btrfs_stack_qgroup_limit_max_exclusive
-				     (limit);
-				a4 = btrfs_stack_qgroup_limit_rsv_referenced
-				     (limit);
-				a5 = btrfs_stack_qgroup_limit_rsv_exclusive
-				     (limit);
-				update_qgroup(qgroup_lookup,
-					   btrfs_search_header_offset(sh), 0,
-					   0, 0, 0, 0, a1, a2, a3, a4, a5,
-					   NULL, NULL);
+					(args.buf + off);
+
+				update_qgroup_limit(qgroup_lookup, qgroupid,
+						    limit);
 			} else if (btrfs_search_header_type(sh)
 				   == BTRFS_QGROUP_RELATION_KEY) {
-				if (btrfs_search_header_offset(sh)
-				    < btrfs_search_header_objectid(sh))
-					goto skip;
-				bq = qgroup_tree_search(qgroup_lookup,
-					btrfs_search_header_offset(sh));
-				if (!bq)
-					goto skip;
-				bq1 = qgroup_tree_search(qgroup_lookup,
-					 btrfs_search_header_objectid(sh));
-				if (!bq1)
+				qgroupid = btrfs_search_header_offset(sh);
+				qgroupid1 = btrfs_search_header_objectid(sh);
+
+				if (qgroupid < qgroupid1)
 					goto skip;
-				update_qgroup(qgroup_lookup,
-					   btrfs_search_header_offset(sh), 0,
-					   0, 0, 0, 0, 0, 0, 0, 0, 0, bq, bq1);
+
+				update_qgroup_relation(qgroup_lookup, qgroupid,
+						       qgroupid1);
 			} else
 				goto done;
 skip:
-- 
2.14.3

[PATCH 3/3] btrfs-progs: qgroup: cleanup __qgroup_search, no functional change

[Lu Fengqi]

1. Use goto instead of while (1) to reduce the level of indentation
2. Replace the if statement with the switch statement
3. move the BTRFS_IOC_TREE_SEARCH ioctl error handler to __qgroup_search

Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
---
 cmds-qgroup.c |   4 --
 qgroup.c      | 152 +++++++++++++++++++++++++++++++---------------------------
 2 files changed, 81 insertions(+), 75 deletions(-)

diff --git a/cmds-qgroup.c b/cmds-qgroup.c
index 38382ea9..d07bb0c0 100644
--- a/cmds-qgroup.c
+++ b/cmds-qgroup.c
@@ -399,10 +399,6 @@ static int cmd_qgroup_show(int argc, char **argv)
 					qgroupid);
 	}
 	ret = btrfs_show_qgroups(fd, filter_set, comparer_set);
-	if (ret == -ENOENT)
-		error("can't list qgroups: quotas not enabled");
-	else if (ret < 0)
-		error("can't list qgroups: %s", strerror(-ret));
 	close_file_or_dir(fd, dirstream);
 
 out:
diff --git a/qgroup.c b/qgroup.c
index 10059e71..b4602b2e 100644
--- a/qgroup.c
+++ b/qgroup.c
@@ -1046,8 +1046,10 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 	struct btrfs_ioctl_search_header *sh;
 	unsigned long off = 0;
 	unsigned int i;
+	struct btrfs_qgroup_status_item *si;
 	struct btrfs_qgroup_info_item *info;
 	struct btrfs_qgroup_limit_item *limit;
+	u64 flags;
 	u64 qgroupid;
 	u64 qgroupid1;
 
@@ -1063,85 +1065,93 @@ static int __qgroups_search(int fd, struct qgroup_lookup *qgroup_lookup)
 
 	qgroup_lookup_init(qgroup_lookup);
 
-	while (1) {
-		ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
-		if (ret < 0)
-			return -errno;
+next:
+	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
+	if (ret < 0) {
+		if (errno == ENOENT) {
+			error("can't list qgroups: quotas not enabled");
+			ret = -ENOTTY;
+		} else {
+			error("can't list qgroups: %s", strerror(errno));
+			ret = -errno;
+		}
 
-		/* the ioctl returns the number of item it found in nr_items */
-		if (sk->nr_items == 0)
+		return ret;
+	}
+
+	/* the ioctl returns the number of item it found in nr_items */
+	if (sk->nr_items == 0)
+		return ret;
+
+	off = 0;
+	/*
+	 * for each item, pull the key out of the header and then
+	 * read the root_ref item it contains
+	 */
+	for (i = 0; i < sk->nr_items; i++) {
+		sh = (struct btrfs_ioctl_search_header *)(args.buf + off);
+		off += sizeof(*sh);
+
+		switch (btrfs_search_header_type(sh)) {
+		case BTRFS_QGROUP_STATUS_KEY:
+			si = (struct btrfs_qgroup_status_item *)
+				(args.buf + off);
+			flags = btrfs_stack_qgroup_status_flags(si);
+
+			print_status_flag_warning(flags);
 			break;
+		case BTRFS_QGROUP_INFO_KEY:
+			qgroupid = btrfs_search_header_offset(sh);
+			info = (struct btrfs_qgroup_info_item *)
+			       (args.buf + off);
 
-		off = 0;
-		/*
-		 * for each item, pull the key out of the header and then
-		 * read the root_ref item it contains
-		 */
-		for (i = 0; i < sk->nr_items; i++) {
-			sh = (struct btrfs_ioctl_search_header *)(args.buf +
-								  off);
-			off += sizeof(*sh);
-
-			if (btrfs_search_header_type(sh)
-			    == BTRFS_QGROUP_STATUS_KEY) {
-				struct btrfs_qgroup_status_item *si;
-				u64 flags;
-
-				si = (struct btrfs_qgroup_status_item *)
-				     (args.buf + off);
-				flags = btrfs_stack_qgroup_status_flags(si);
-				print_status_flag_warning(flags);
-			} else if (btrfs_search_header_type(sh)
-				   == BTRFS_QGROUP_INFO_KEY) {
-				qgroupid = btrfs_search_header_offset(sh);
-				info = (struct btrfs_qgroup_info_item *)
-				       (args.buf + off);
-
-				update_qgroup_info(qgroup_lookup, qgroupid,
-						   info);
-			} else if (btrfs_search_header_type(sh)
-				   == BTRFS_QGROUP_LIMIT_KEY) {
-				qgroupid = btrfs_search_header_offset(sh);
-				limit = (struct btrfs_qgroup_limit_item *)
-					(args.buf + off);
-
-				update_qgroup_limit(qgroup_lookup, qgroupid,
-						    limit);
-			} else if (btrfs_search_header_type(sh)
-				   == BTRFS_QGROUP_RELATION_KEY) {
-				qgroupid = btrfs_search_header_offset(sh);
-				qgroupid1 = btrfs_search_header_objectid(sh);
-
-				if (qgroupid < qgroupid1)
-					goto skip;
-
-				update_qgroup_relation(qgroup_lookup, qgroupid,
-						       qgroupid1);
-			} else
-				goto done;
-skip:
-			off += btrfs_search_header_len(sh);
-
-			/*
-			 * record the mins in sk so we can make sure the
-			 * next search doesn't repeat this root
-			 */
-			sk->min_type = btrfs_search_header_type(sh);
-			sk->min_offset = btrfs_search_header_offset(sh);
-			sk->min_objectid = btrfs_search_header_objectid(sh);
+			ret = update_qgroup_info(qgroup_lookup, qgroupid, info);
+			break;
+		case BTRFS_QGROUP_LIMIT_KEY:
+			qgroupid = btrfs_search_header_offset(sh);
+			limit = (struct btrfs_qgroup_limit_item *)
+				(args.buf + off);
+
+			ret = update_qgroup_limit(qgroup_lookup, qgroupid,
+						  limit);
+			break;
+		case BTRFS_QGROUP_RELATION_KEY:
+			qgroupid = btrfs_search_header_offset(sh);
+			qgroupid1 = btrfs_search_header_objectid(sh);
+
+			if (qgroupid < qgroupid1)
+				break;
+
+			ret = update_qgroup_relation(qgroup_lookup, qgroupid,
+						     qgroupid1);
+			break;
+		default:
+			return ret;
 		}
-		sk->nr_items = 4096;
+
+		if (ret)
+			return ret;
+
+		off += btrfs_search_header_len(sh);
+
 		/*
-		 * this iteration is done, step forward one qgroup for the next
-		 * ioctl
+		 * record the mins in sk so we can make sure the
+		 * next search doesn't repeat this root
 		 */
-		if (sk->min_offset < (u64)-1)
-			sk->min_offset++;
-		else
-			break;
+		sk->min_type = btrfs_search_header_type(sh);
+		sk->min_offset = btrfs_search_header_offset(sh);
+		sk->min_objectid = btrfs_search_header_objectid(sh);
+	}
+	sk->nr_items = 4096;
+	/*
+	 * this iteration is done, step forward one qgroup for the next
+	 * ioctl
+	 */
+	if (sk->min_offset < (u64)-1) {
+		sk->min_offset++;
+		goto next;
 	}
 
-done:
 	return ret;
 }
 
-- 
2.14.3

Re: [PATCH 3/3] btrfs-progs: qgroup: cleanup __qgroup_search, no functional change

[David Sterba]

On Tue, Oct 31, 2017 at 05:13:45PM +0800, Lu Fengqi wrote:
> 1. Use goto instead of while (1) to reduce the level of indentation

I'd rather avoid this goto pattern in new code, using while is ok. If
the indentation depth becomes problem, then the inner code should be
moved to a helper.

> 2. Replace the if statement with the switch statement

This is good.

> 3. move the BTRFS_IOC_TREE_SEARCH ioctl error handler to __qgroup_search

This one as well, but please make them into separate patches, the code
is being moved and changed at the same time. This makes the review
harder.

Re: [PATCH 0/3] add_qgroup/update_qgroup/__qgroup_search cleanup

[David Sterba]

On Tue, Oct 31, 2017 at 05:13:42PM +0800, Lu Fengqi wrote:
> Patch 1 just remove the redundant function add_qgroup.
> Patch 2 separate QGROUP_INFO/QGROUP_LIMIT/QGROUP_RELATION process from
> the update_qgroup.
> Patch 3 mainly the coding style adjustment.

This per-patch description is fine, and not always necessary if the
patch subjects and changelogs are sufficient. The cover letter could be
some overview what are the patches addressing.

> 
> Lu Fengqi (3):
>   btrfs-progs: qgroup: cleanup the redundant function add_qgroup
>   btrfs-progs: qgroup: split update_qgroup to reduce arguments

1 and 2 applied, the cleanups look very good, thanks.

>   btrfs-progs: qgroup: cleanup __qgroup_search, no functional change

Not applied, please see the comments.

Re: [PATCH 3/3] btrfs-progs: qgroup: cleanup __qgroup_search, no functional change

[Lu Fengqi]

On Thu, Nov 09, 2017 at 05:51:48PM +0100, David Sterba wrote:
>On Tue, Oct 31, 2017 at 05:13:45PM +0800, Lu Fengqi wrote:
>> 1. Use goto instead of while (1) to reduce the level of indentation
>
>I'd rather avoid this goto pattern in new code, using while is ok. If
>the indentation depth becomes problem, then the inner code should be
>moved to a helper.

Make sense.

>
>> 2. Replace the if statement with the switch statement
>
>This is good.
>
>> 3. move the BTRFS_IOC_TREE_SEARCH ioctl error handler to __qgroup_search
>
>This one as well, but please make them into separate patches, the code
>is being moved and changed at the same time. This makes the review
>harder.
>
>

Ok, I'll split the patch and resend it later.

-- 
Thanks,
Lu