Staging: lustre: Fix line length exceeding 80 characters

PATCH In review MonamAgarwal 5 participants 12 messages Jan 5, 2014 – Jan 14, 2014 1 message not in the archive Staging

Share & tools

Joe Perches

2014-01-05 20:31 UTC #1 lore raw

Quoted 3 lines · MonamAgarwal

This patch fixes the following checkpatch.pl warning in lustre/ldlm/interval_tree.c
WARNING: line over 80 characters in the file

[]

drivers/staging/lustre/lustre/ldlm/interval_tree.c

Quoted 1 line

drivers/staging/lustre/lustre/ldlm/interval_tree.c +0 -0

[]

drivers/staging/lustre/lustre/ldlm/interval_tree.c static void interval_erase_color(struct interval_node *node, · line 433

Quoted 7 lines

drivers/staging/lustre/lustre/ldlm/interval_tree.c +2 -1

@@ -429,7 +429,8 @@ static void interval_erase_color(struct interval_node *node,
 					struct interval_node *o_left;
 					o_left = tmp->in_left;
 					if (o_left)
-						o_left->in_color = INTERVAL_BLACK;
+						o_left->in_color =
+							INTERVAL_BLACK;

Likely this function would be better off with some refactoring instead of straining to fit 80 cols.

Greg KH

maintainer

2014-01-08 23:54 UTC #2 lore raw

Quoted 7 lines · Joe Perches

Quoted 3 lines · MonamAgarwal
This patch fixes the following checkpatch.pl warning in lustre/ldlm/interval_tree.c
WARNING: line over 80 characters in the file
[]
drivers/staging/lustre/lustre/ldlm/interval_tree.c
Quoted 1 line
drivers/staging/lustre/lustre/ldlm/interval_tree.c +0 -0
[]

Quoted 10 lines

drivers/staging/lustre/lustre/ldlm/interval_tree.c static void interval_erase_color(struct interval_node *node, · line 433
Quoted 7 lines
drivers/staging/lustre/lustre/ldlm/interval_tree.c +2 -1
@@ -429,7 +429,8 @@ static void interval_erase_color(struct interval_node *node,
 					struct interval_node *o_left;
 					o_left = tmp->in_left;
 					if (o_left)
-						o_left->in_color = INTERVAL_BLACK;
+						o_left->in_color =
+							INTERVAL_BLACK;
Likely this function would be better off with some refactoring instead of straining to fit 80 cols.

I agree, if you are doing things like this, that's a huge hint that the code needs fixing.

thanks,

greg k-h

Monam Agarwal

author

2014-01-11 11:00 UTC #3 lore raw

PATCH v3 1/2 Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

In reply to MonamAgarwal

The function interval_erase_color() in lustre/ldlm/interval_tree.c is too long and can be refactored. Most of the statements are same for if and else conditions. I am passing a new variable n based on which the differences are recognised.

This fixes the following checkpatch.pl warning in
lustre/ldlm/interval_tree.c
WARNING: line over 80 characters in the file

Signed-off-by Monam Agarwal <[email protected]>

---

Changes since version 1:
* Incorrect fixing
* Incorrect Signed-off-by line

Changes since version 2:
* Removed new line character

 drivers/staging/lustre/lustre/ldlm/interval_tree.c |  128 +++++++++++---------
 1 file changed, 69 insertions(+), 59 deletions(-)

drivers/staging/lustre/lustre/ldlm/interval_tree.c +69 ~~-59~~

index 1de1d8e..7c956de 100644
@@ -404,75 +404,85 @@ static inline int node_is_black_or_0(struct interval_node *node)
 	return !node || node_is_black(node);
 }
 
+static int interval_set_color(struct interval_node *node,
+			      struct interval_node *parent,
+			      struct interval_node *root,
+			      int n)
+{
+	struct interval_node *tmp;
+	if (n == 0)
+		tmp = parent->in_right;
+	else
+		tmp = parent->in_left;
+	if (node_is_red(tmp)) {
+		tmp->in_color = INTERVAL_BLACK;
+		parent->in_color = INTERVAL_RED;
+		if (n == 0) {
+			__rotate_left(parent, root);
+			tmp = parent->in_right;
+		} else {
+			__rotate_right(parent, root);
+			tmp = parent->in_left;
+		}
+	}
+	if (node_is_black_or_0(tmp->in_left) &&
+	    node_is_black_or_0(tmp->in_right)) {
+		tmp->in_color = INTERVAL_RED;
+		node = parent;
+		parent = node->in_parent;
+	} else {
+		if (n == 0) {
+			if (node_is_black_or_0(tmp->in_right)) {
+				struct interval_node *o_left;
+				o_left = tmp->in_left;
+				if (o_left)
+					o_left->in_color = INTERVAL_BLACK;
+				tmp->in_color = INTERVAL_RED;
+				__rotate_right(tmp, root);
+				tmp = parent->in_right;
+			}
+			tmp->in_color = parent->in_color;
+			parent->in_color = INTERVAL_BLACK;
+			if (tmp->in_right)
+				tmp->in_right->in_color = INTERVAL_BLACK;
+			__rotate_left(parent, root);
+			node = *root;
+			return 1;
+		} else {
+			if (node_is_black_or_0(tmp->in_left)) {
+				struct interval_node *o_right;
+				o_right = tmp->in_right;
+				if (o_right)
+					o_right->in_color = INTERVAL_BLACK;
+				tmp->in_color = INTERVAL_RED;
+				__rotate_left(tmp, root);
+				tmp = parent->in_left;
+			}
+			tmp->in_color = parent->in_color;
+			parent->in_color = INTERVAL_BLACK;
+			if (tmp->in_left)
+				tmp->in_left->in_color = INTERVAL_BLACK;
+			__rotate_right(parent, root);
+			node = *root;
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static void interval_erase_color(struct interval_node *node,
 				 struct interval_node *parent,
 				 struct interval_node **root)
 {
-	struct interval_node *tmp;
 
 	while (node_is_black_or_0(node) && node != *root) {
 		if (parent->in_left == node) {
-			tmp = parent->in_right;
-			if (node_is_red(tmp)) {
-				tmp->in_color = INTERVAL_BLACK;
-				parent->in_color = INTERVAL_RED;
-				__rotate_left(parent, root);
-				tmp = parent->in_right;
-			}
-			if (node_is_black_or_0(tmp->in_left) &&
-			    node_is_black_or_0(tmp->in_right)) {
-				tmp->in_color = INTERVAL_RED;
-				node = parent;
-				parent = node->in_parent;
-			} else {
-				if (node_is_black_or_0(tmp->in_right)) {
-					struct interval_node *o_left;
-					o_left = tmp->in_left;
-					if (o_left)
-						o_left->in_color = INTERVAL_BLACK;
-					tmp->in_color = INTERVAL_RED;
-					__rotate_right(tmp, root);
-					tmp = parent->in_right;
-				}
-				tmp->in_color = parent->in_color;
-				parent->in_color = INTERVAL_BLACK;
-				if (tmp->in_right)
-					tmp->in_right->in_color = INTERVAL_BLACK;
-				__rotate_left(parent, root);
-				node = *root;
+			if (interval_set_color(node, parent, root, 0))
 				break;
-			}
 		} else {
-			tmp = parent->in_left;
-			if (node_is_red(tmp)) {
-				tmp->in_color = INTERVAL_BLACK;
-				parent->in_color = INTERVAL_RED;
-				__rotate_right(parent, root);
-				tmp = parent->in_left;
-			}
-			if (node_is_black_or_0(tmp->in_left) &&
-			    node_is_black_or_0(tmp->in_right)) {
-				tmp->in_color = INTERVAL_RED;
-				node = parent;
-				parent = node->in_parent;
-			} else {
-				if (node_is_black_or_0(tmp->in_left)) {
-					struct interval_node *o_right;
-					o_right = tmp->in_right;
-					if (o_right)
-						o_right->in_color = INTERVAL_BLACK;
-					tmp->in_color = INTERVAL_RED;
-					__rotate_left(tmp, root);
-					tmp = parent->in_left;
-				}
-				tmp->in_color = parent->in_color;
-				parent->in_color = INTERVAL_BLACK;
-				if (tmp->in_left)
-					tmp->in_left->in_color = INTERVAL_BLACK;
-				__rotate_right(parent, root);
-				node = *root;
+
+			if (interval_set_color(node, parent, root, 1))
 				break;
-			}
 		}
 	}
 	if (node)

Signature · 1 line

1.7.9.5

Dan Carpenter

2014-01-11 11:19 UTC #4 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 5 lines · Monam Agarwal

The function interval_erase_color() in lustre/ldlm/interval_tree.c is too long and can be refactored. Most of the statements are same for if and else conditions. I am passing a new variable n based on which the differences are recognised.

"n" stands for number, but what is the it the number of?

regards,
dan carpenter

Monam Agarwal

author

2014-01-11 11:26 UTC #5 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

I took n as a flag to decide whether parent->in_left == node is true or not in the called function.
Should I use some other name for the flag.

Quoted 12 lines · Dan Carpenter

Quoted 5 lines · Monam Agarwal
The function interval_erase_color() in lustre/ldlm/interval_tree.c is too long and can be refactored. Most of the statements are same for if and else conditions. I am passing a new variable n based on which the differences are recognised.
"n" stands for number, but what is the it the number of?
regards,
dan carpenter

Dan Carpenter

2014-01-11 11:40 UTC #6 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 2 lines · Monam Agarwal

I took n as a flag to decide whether parent->in_left == node is true or not in the called function.

So "n" stands for "node"?

Quoted 1 line

Should I use some other name for the flag.

Yes.

regards,
dan carpenter

Monam Agarwal

author

2014-01-11 11:44 UTC #7 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 11 lines · Dan Carpenter

Quoted 2 lines · Monam Agarwal
I took n as a flag to decide whether parent->in_left == node is true or not in the called function.
So "n" stands for "node"?
Quoted 1 line
Should I use some other name for the flag.
Yes.

Will "flag" be a suitable name?

Greg KH

maintainer

2014-01-11 20:33 UTC #8 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 7 lines · Monam Agarwal

Quoted 6 lines · Dan Carpenter
Quoted 2 lines · Monam Agarwal
I took n as a flag to decide whether parent->in_left == node is true or not in the called function.
So "n" stands for "node"?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Should I use some other name for the flag.
Yes.
Will "flag" be a suitable name?

Ick, no. You don't want a "flag" to have to determine what the logic is for a given function. That just causes confusion and makes things really hard to read and understand over time.

This whole function looks like a red/black tree, or something like that. Shouldn't we just be using the in-kernel implementation of this? And if not, then you really need to get the feedback of the code's original authors as you might be changing the algorithm in ways that could cause big problems.

thanks,

greg k-h

A message that is not in the archive (sent off-list or not yet received): [email protected]

Xiong, Jinshan

2014-01-14 08:01 UTC #10 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 25 lines · Andreas

Begin forwarded message:
Quoted 21 lines
From: Greg KH <[email protected]>
Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
Date: January 11, 2014 at 1:33:58 PM MST
To: Monam Agarwal <[email protected]>
Cc: Dan Carpenter <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, Rashika Kheria <[email protected]>
Quoted 14 lines · Monam Agarwal
Quoted 11 lines · Dan Carpenter
Quoted 2 lines · Monam Agarwal
I took n as a flag to decide whether parent->in_left == node is true or not in the called function.
So "n" stands for "node"?
Quoted 1 line
Should I use some other name for the flag.
Yes.
Will "flag" be a suitable name?

I’d suggest `bool is_right_child’.

I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.

Jinshan

Quoted 26 lines

Quoted 11 lines
Ick, no. You don't want a "flag" to have to determine what the logic is for a given function. That just causes confusion and makes things really hard to read and understand over time.
This whole function looks like a red/black tree, or something like that. Shouldn't we just be using the in-kernel implementation of this? And if not, then you really need to get the feedback of the code's original authors as you might be changing the algorithm in ways that could cause big problems.
Quoted 3 lines
thanks,
greg k-h
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel Corporation

Monam Agarwal

author

2014-01-14 11:09 UTC #11 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

Quoted 24 lines · Jinshan

Quoted 21 lines · Andreas
Begin forwarded message:
Quoted 17 lines
From: Greg KH <[email protected]>
Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
Date: January 11, 2014 at 1:33:58 PM MST
To: Monam Agarwal <[email protected]>
Cc: Dan Carpenter <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, Rashika Kheria <[email protected]>
Quoted 10 lines · Monam Agarwal
Quoted 9 lines · Dan Carpenter
Quoted 2 lines · Monam Agarwal
I took n as a flag to decide whether parent->in_left == node is true or not in the called function.
So "n" stands for "node"?
Quoted 1 line
Should I use some other name for the flag.

Quoted 9 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Yes.
Will "flag" be a suitable name?
I’d suggest `bool is_right_child’.
I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.

I am using tree from
http://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/.
There is no lustre/tests folder in my tree and no file named it_test.c.
Kindly let me know the tree you are working from.

Quoted 29 lines

Jinshan
Quoted 26 lines
Quoted 11 lines
Ick, no. You don't want a "flag" to have to determine what the logic is for a given function. That just causes confusion and makes things really hard to read and understand over time.
This whole function looks like a red/black tree, or something like that. Shouldn't we just be using the in-kernel implementation of this? And if not, then you really need to get the feedback of the code's original authors as you might be changing the algorithm in ways that could cause big problems.
Quoted 3 lines
thanks,
greg k-h
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel Corporation

Xiong, Jinshan

2014-01-14 17:44 UTC #12 lore raw

Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c

On Jan 14, 2014, at 3:09 AM, Monam Agarwal <[email protected]<mailto:[email protected]>> wrote:

On Tue, Jan 14, 2014 at 1:31 PM, Xiong, Jinshan <[email protected]<mailto:[email protected]>> wrote:

On Jan 13, 2014, at 11:56 PM, Dilger, Andreas <[email protected]<mailto:[email protected]>> wrote:

Begin forwarded message:

From: Greg KH <[email protected]<mailto:[email protected]>>
Subject: Re: [PATCH v3 1/2] Staging: lustre: Refactor the function interval_erase_color() in /lustre/ldlm/interval_tree.c
Date: January 11, 2014 at 1:33:58 PM MST
To: Monam Agarwal <[email protected]<mailto:[email protected]>>
Cc: Dan Carpenter <[email protected]<mailto:[email protected]>>, <[email protected]<mailto:[email protected]>>, <[email protected]<mailto:[email protected]>>, <[email protected]<mailto:[email protected]>>, <[email protected]<mailto:[email protected]>>, Rashika Kheria <[email protected]<mailto:[email protected]>>

On Sat, Jan 11, 2014 at 05:14:35PM +0530, Monam Agarwal wrote:
On Sat, Jan 11, 2014 at 5:09 PM, Dan Carpenter <[email protected]<mailto:[email protected]>> wrote:
On Sat, Jan 11, 2014 at 04:56:44PM +0530, Monam Agarwal wrote:
I took n as a flag to decide whether parent->in_left == node is true
or not in the called function.

So "n" stands for "node"?

Should I use some other name for the flag.

Yes.

Will "flag" be a suitable name?

I’d suggest `bool is_right_child’.

I’ve checked the patch and it looks good. There exists a unit test case for interval tree under lustre/tests/ named it_test.c, please compile it and verify your change.

I’m using git tree git://git.hpdd.intel.com/fs/lustre-release.git<http://git.hpdd.intel.com/?p=fs/lustre-release.git>

there are also some useful information about compiling and running the code: https://wiki.hpdd.intel.com/display/PUB/Testing+a+Lustre+filesystem, usually I download a patched kernel rpm so that I can save a lot of time from building the kernel myself. Go to build.whamcloud.com<http://build.whamcloud.com> and look for lustre-master, click in and choose server side package based on your distro.

If you’re interested in contributing code to lustre, here are some useful links about creating the patch, review the patch, pass regression test, etc: https://wiki.hpdd.intel.com/display/PUB/Lustre+Development

Jinshan

Ick, no. You don't want a "flag" to have to determine what the logic is for a given function. That just causes confusion and makes things really hard to read and understand over time.

thanks,

greg k-h

Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel Corporation