Skip to contentSkip to conversation

Fixed signedness check

PATCH In review Guillermo O. Freschi 3 participants 6 messages Dec 26, 2016 – Jan 16, 2017 Staging

lore
Share & tools
lore

Guillermo O. Freschi

author
#0 lore raw

PATCH 1/2 Fixed signedness check

Was unsigned int, but enums are signed.

Signed-off-by Guillermo O. Freschi <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +1 -1
index a4a291acb659..f4cbc89b4f24 100644
@@ -1148,7 +1148,7 @@ static int lock_matches(struct ldlm_lock *lock, struct lock_match_data *data)
 	return INTERVAL_ITER_STOP;
 }
 
-static unsigned int itree_overlap_cb(struct interval_node *in, void *args)
+static enum interval_iter itree_overlap_cb(struct interval_node *in, void *args)
 {
 	struct ldlm_interval *node = to_ldlm_interval(in);
 	struct lock_match_data *data = args;
Signature · 1 line
2.11.0
author
#1 lore raw

PATCH 2/2 Style fixes

Missing braces on if statement; misaligned parameter.

Signed-off-by Guillermo O. Freschi <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +7 -5
index f4cbc89b4f24..a23e7ada3891 100644
@@ -1024,11 +1024,11 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 	if (work_list && lock->l_completion_ast)
 		ldlm_add_ast_work_item(lock, NULL, work_list);
 
-	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS)
+	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS) {
 		ldlm_grant_lock_with_skiplist(lock);
-	else if (res->lr_type == LDLM_EXTENT)
+	} else if (res->lr_type == LDLM_EXTENT) {
 		ldlm_extent_add_lock(res, lock);
-	else if (res->lr_type == LDLM_FLOCK) {
+	} else if (res->lr_type == LDLM_FLOCK) {
 		/*
 		 * We should not add locks to granted list in the following cases:
 		 * - this is an UNLOCK but not a real lock;

@@ -1040,8 +1040,9 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 		    ldlm_is_test_lock(lock) || ldlm_is_flock_deadlock(lock))
 			return;
 		ldlm_resource_add_lock(res, &res->lr_granted, lock);
-	} else
+	} else {
 		LBUG();
+	}
 
 	ldlm_pool_add(&ldlm_res_to_ns(res)->ns_pool, lock);
 }

@@ -1481,7 +1482,8 @@ int ldlm_fill_lvb(struct ldlm_lock *lock, struct req_capsule *pill,
 							lustre_swab_ost_lvb_v1);
 			else
 				lvb = req_capsule_server_sized_swab_get(pill,
-						&RMF_DLM_LVB, size,
+									&RMF_DLM_LVB,
+									size,
 						lustre_swab_ost_lvb_v1);
 			if (unlikely(!lvb)) {
 				LDLM_ERROR(lock, "no LVB");
Signature · 1 line
2.11.0
#2 lore raw

Re: [lustre-devel] [PATCH 2/2] Style fixes

Quoted 19 lines · Guillermo O. Freschi

Missing braces on if statement; misaligned parameter.

Signed-off-by Guillermo O. Freschi <[email protected]>

---
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 12 +++++++-----

1 file changed, 7 insertions(+), 5 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +0 -1
index f4cbc89b4f24..a23e7ada3891 100644
@@ -1024,11 +1024,11 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 	if (work_list && lock->l_completion_ast)
 		ldlm_add_ast_work_item(lock, NULL, work_list);
 
-	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS)

drivers/staging/lustre/lustre/ldlm/ldlm_lock.c void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list) · line 1034

Quoted 10 lines
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +3 -2
+	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS) {
 		ldlm_grant_lock_with_skiplist(lock);
-	else if (res->lr_type == LDLM_EXTENT)
+	} else if (res->lr_type == LDLM_EXTENT) {
 		ldlm_extent_add_lock(res, lock);
-	else if (res->lr_type == LDLM_FLOCK) {
+	} else if (res->lr_type == LDLM_FLOCK) {
 		/*
 		 * We should not add locks to granted list in the following cases:
 		 * - this is an UNLOCK but not a real lock;

This part is fine.

drivers/staging/lustre/lustre/ldlm/ldlm_lock.c void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list) · line 1048

Quoted 11 lines
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +2 -1
@@ -1040,8 +1040,9 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 		    ldlm_is_test_lock(lock) || ldlm_is_flock_deadlock(lock))
 			return;
 		ldlm_resource_add_lock(res, &res->lr_granted, lock);
-	} else
+	} else {
 		LBUG();
+	}
 
 	ldlm_pool_add(&ldlm_res_to_ns(res)->ns_pool, lock);
 }

Fine.

drivers/staging/lustre/lustre/ldlm/ldlm_lock.c int ldlm_fill_lvb(struct ldlm_lock *lock, struct req_capsule *pill, · line 1487

Quoted 8 lines
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +2 -1
@@ -1481,7 +1482,8 @@ int ldlm_fill_lvb(struct ldlm_lock *lock, struct req_capsule *pill,
 							lustre_swab_ost_lvb_v1);
 			else
 				lvb = req_capsule_server_sized_swab_get(pill,
-						&RMF_DLM_LVB, size,
+									&RMF_DLM_LVB,
+									size,
 						lustre_swab_ost_lvb_v1);

Not so keen on this one, since "lustre_swab_ost_lvb_v1" can't fit after the '(' of the original function line, there isn't any benefit to aligning the other two arguments but not that one. Better to keep the indentation the same and save one line of whitespace.

Can you please resubmit without this hunk.

Cheers, Andreas

author
#3 lore raw

PATCH v2 1/1 Style fixes

In reply to Guillermo O. Freschi

Missing braces on if statement.

Signed-off-by Guillermo O. Freschi <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +5 -4
index f4cbc89b4f24..b66bc02646f1 100644
@@ -1024,11 +1024,11 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 	if (work_list && lock->l_completion_ast)
 		ldlm_add_ast_work_item(lock, NULL, work_list);
 
-	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS)
+	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS) {
 		ldlm_grant_lock_with_skiplist(lock);
-	else if (res->lr_type == LDLM_EXTENT)
+	} else if (res->lr_type == LDLM_EXTENT) {
 		ldlm_extent_add_lock(res, lock);
-	else if (res->lr_type == LDLM_FLOCK) {
+	} else if (res->lr_type == LDLM_FLOCK) {
 		/*
 		 * We should not add locks to granted list in the following cases:
 		 * - this is an UNLOCK but not a real lock;

@@ -1040,8 +1040,9 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 		    ldlm_is_test_lock(lock) || ldlm_is_flock_deadlock(lock))
 			return;
 		ldlm_resource_add_lock(res, &res->lr_granted, lock);
-	} else
+	} else {
 		LBUG();
+	}
 
 	ldlm_pool_add(&ldlm_res_to_ns(res)->ns_pool, lock);
 }
Signature · 1 line
2.11.0
Reviewed-by on v2 1/1
#4 lore raw

Re: [PATCH v2] Style fixes

Quoted 5 lines · Guillermo O. Freschi

Missing braces on if statement.

Signed-off-by Guillermo O. Freschi <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
Reviewed-by Oleg Drokin <[email protected]>
Quoted 36 lines

---
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 9 +++++----

1 file changed, 5 insertions(+), 4 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +5 -4
index f4cbc89b4f24..b66bc02646f1 100644
@@ -1024,11 +1024,11 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 	if (work_list && lock->l_completion_ast)
 		ldlm_add_ast_work_item(lock, NULL, work_list);
 
-	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS)
+	if (res->lr_type == LDLM_PLAIN || res->lr_type == LDLM_IBITS) {
 		ldlm_grant_lock_with_skiplist(lock);
-	else if (res->lr_type == LDLM_EXTENT)
+	} else if (res->lr_type == LDLM_EXTENT) {
 		ldlm_extent_add_lock(res, lock);
-	else if (res->lr_type == LDLM_FLOCK) {
+	} else if (res->lr_type == LDLM_FLOCK) {
 		/*
 		 * We should not add locks to granted list in the following cases:
 		 * - this is an UNLOCK but not a real lock;

@@ -1040,8 +1040,9 @@ void ldlm_grant_lock(struct ldlm_lock *lock, struct list_head *work_list)
 		    ldlm_is_test_lock(lock) || ldlm_is_flock_deadlock(lock))
 			return;
 		ldlm_resource_add_lock(res, &res->lr_granted, lock);
-	} else
+	} else {
 		LBUG();
+	}
 
 	ldlm_pool_add(&ldlm_res_to_ns(res)->ns_pool, lock);
 }
Signature · 1 line
2.11.0
Reviewed-by on v1 1/2
#5 lore raw
In reply to Guillermo O. Freschi
Quoted 2 lines · Guillermo O. Freschi

Was unsigned int, but enums are signed.

Interesting that GCC didn't complain that the itree_overlap_cb() function signature didn't match the argument prototype for interval_search():

typedef enum interval_iter (*interval_callback_t)(struct interval_node *node,

                                                    void *args);

In the end it is a somewhat cosmetic fix, because the only enum values (1, 2) were unsigned, but good to be consistent. Thanks for the fix.

Quoted 1 line
Signed-off-by Guillermo O. Freschi <[email protected]>
Reviewed-by Andreas Dilger <[email protected]>
Quoted 24 lines

---
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +1 -1
index a4a291acb659..f4cbc89b4f24 100644
@@ -1148,7 +1148,7 @@ static int lock_matches(struct ldlm_lock *lock, struct lock_match_data *data)
 	return INTERVAL_ITER_STOP;
 }
 
-static unsigned int itree_overlap_cb(struct interval_node *in, void *args)
+static enum interval_iter itree_overlap_cb(struct interval_node *in, void *args)
 {
 	struct ldlm_interval *node = to_ldlm_interval(in);
 	struct lock_match_data *data = args;
Signature · 6 lines
2.11.0

_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org