ocfs2: optimize error handling in dlm_request_join
diff mbox

Message ID 55D5BEE8.8070001@huawei.com
State New
Headers show

Commit Message

Norton.Zhu Aug. 20, 2015, 11:50 a.m. UTC
Currently error handling in dlm_request_join is a little obscure.
So optimize it to promote readability.

Signed-off-by: Norton.Zhu <norton.zhu@huawei.com>
---
 fs/ocfs2/dlm/dlmdomain.c | 69 ++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 32 deletions(-)

Comments

Joseph Qi Aug. 20, 2015, 12:42 p.m. UTC | #1
On 2015/8/20 19:50, Norton.Zhu wrote:
> Currently error handling in dlm_request_join is a little obscure.
> So optimize it to promote readability.
> 
> Signed-off-by: Norton.Zhu <norton.zhu@huawei.com>
Reviewed-by: Joseph Qi <joseph.qi@huawei.com>

> ---
>  fs/ocfs2/dlm/dlmdomain.c | 69 ++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> index 7df88a6..af4f7aa 100644
> --- a/fs/ocfs2/dlm/dlmdomain.c
> +++ b/fs/ocfs2/dlm/dlmdomain.c
> @@ -1465,39 +1465,44 @@ static int dlm_request_join(struct dlm_ctxt *dlm,
>  	if (status == -ENOPROTOOPT) {
>  		status = 0;
>  		*response = JOIN_OK_NO_MAP;
> -	} else if (packet.code == JOIN_DISALLOW ||
> -		   packet.code == JOIN_OK_NO_MAP) {
> -		*response = packet.code;
> -	} else if (packet.code == JOIN_PROTOCOL_MISMATCH) {
> -		mlog(ML_NOTICE,
> -		     "This node requested DLM locking protocol %u.%u and "
> -		     "filesystem locking protocol %u.%u.  At least one of "
> -		     "the protocol versions on node %d is not compatible, "
> -		     "disconnecting\n",
> -		     dlm->dlm_locking_proto.pv_major,
> -		     dlm->dlm_locking_proto.pv_minor,
> -		     dlm->fs_locking_proto.pv_major,
> -		     dlm->fs_locking_proto.pv_minor,
> -		     node);
> -		status = -EPROTO;
> -		*response = packet.code;
> -	} else if (packet.code == JOIN_OK) {
> -		*response = packet.code;
> -		/* Use the same locking protocol as the remote node */
> -		dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
> -		dlm->fs_locking_proto.pv_minor = packet.fs_minor;
> -		mlog(0,
> -		     "Node %d responds JOIN_OK with DLM locking protocol "
> -		     "%u.%u and fs locking protocol %u.%u\n",
> -		     node,
> -		     dlm->dlm_locking_proto.pv_major,
> -		     dlm->dlm_locking_proto.pv_minor,
> -		     dlm->fs_locking_proto.pv_major,
> -		     dlm->fs_locking_proto.pv_minor);
>  	} else {
> -		status = -EINVAL;
> -		mlog(ML_ERROR, "invalid response %d from node %u\n",
> -		     packet.code, node);
> +		*response = packet.code;
> +		switch (packet.code) {
> +		case JOIN_DISALLOW:
> +		case JOIN_OK_NO_MAP:
> +			break;
> +		case JOIN_PROTOCOL_MISMATCH:
> +			mlog(ML_NOTICE,
> +			     "This node requested DLM locking protocol %u.%u and "
> +			     "filesystem locking protocol %u.%u.  At least one of "
> +			     "the protocol versions on node %d is not compatible, "
> +			     "disconnecting\n",
> +			     dlm->dlm_locking_proto.pv_major,
> +			     dlm->dlm_locking_proto.pv_minor,
> +			     dlm->fs_locking_proto.pv_major,
> +			     dlm->fs_locking_proto.pv_minor,
> +			     node);
> +			status = -EPROTO;
> +			break;
> +		case JOIN_OK:
> +			/* Use the same locking protocol as the remote node */
> +			dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
> +			dlm->fs_locking_proto.pv_minor = packet.fs_minor;
> +			mlog(0,
> +			     "Node %d responds JOIN_OK with DLM locking protocol "
> +			     "%u.%u and fs locking protocol %u.%u\n",
> +			     node,
> +			     dlm->dlm_locking_proto.pv_major,
> +			     dlm->dlm_locking_proto.pv_minor,
> +			     dlm->fs_locking_proto.pv_major,
> +			     dlm->fs_locking_proto.pv_minor);
> +			break;
> +		default:
> +			status = -EINVAL;
> +			mlog(ML_ERROR, "invalid response %d from node %u\n",
> +			     packet.code, node);
> +			break;
> +		}
>  	}
> 
>  	mlog(0, "status %d, node %d response is %d\n", status, node,
>
Srinivas Eeda Aug. 20, 2015, 4:56 p.m. UTC | #2
On 08/20/2015 04:50 AM, Norton.Zhu wrote:
> Currently error handling in dlm_request_join is a little obscure.
> So optimize it to promote readability.
>
> Signed-off-by: Norton.Zhu <norton.zhu@huawei.com>
> ---
>   fs/ocfs2/dlm/dlmdomain.c | 69 ++++++++++++++++++++++++++----------------------
>   1 file changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
> index 7df88a6..af4f7aa 100644
> --- a/fs/ocfs2/dlm/dlmdomain.c
> +++ b/fs/ocfs2/dlm/dlmdomain.c
> @@ -1465,39 +1465,44 @@ static int dlm_request_join(struct dlm_ctxt *dlm,
>   	if (status == -ENOPROTOOPT) {
>   		status = 0;
>   		*response = JOIN_OK_NO_MAP;
> -	} else if (packet.code == JOIN_DISALLOW ||
> -		   packet.code == JOIN_OK_NO_MAP) {
> -		*response = packet.code;
> -	} else if (packet.code == JOIN_PROTOCOL_MISMATCH) {
> -		mlog(ML_NOTICE,
> -		     "This node requested DLM locking protocol %u.%u and "
> -		     "filesystem locking protocol %u.%u.  At least one of "
> -		     "the protocol versions on node %d is not compatible, "
> -		     "disconnecting\n",
> -		     dlm->dlm_locking_proto.pv_major,
> -		     dlm->dlm_locking_proto.pv_minor,
> -		     dlm->fs_locking_proto.pv_major,
> -		     dlm->fs_locking_proto.pv_minor,
> -		     node);
> -		status = -EPROTO;
> -		*response = packet.code;
> -	} else if (packet.code == JOIN_OK) {
> -		*response = packet.code;
> -		/* Use the same locking protocol as the remote node */
> -		dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
> -		dlm->fs_locking_proto.pv_minor = packet.fs_minor;
> -		mlog(0,
> -		     "Node %d responds JOIN_OK with DLM locking protocol "
> -		     "%u.%u and fs locking protocol %u.%u\n",
> -		     node,
> -		     dlm->dlm_locking_proto.pv_major,
> -		     dlm->dlm_locking_proto.pv_minor,
> -		     dlm->fs_locking_proto.pv_major,
> -		     dlm->fs_locking_proto.pv_minor);
>   	} else {
> -		status = -EINVAL;
> -		mlog(ML_ERROR, "invalid response %d from node %u\n",
> -		     packet.code, node);
> +		*response = packet.code;
Norton, it looks much better :)

one minor comment. we don't want to reset "*response" with packet.code 
if it's unrecognized. We should leave the value to JOIN_DISALLOW;

rest looks good.

> +		switch (packet.code) {
> +		case JOIN_DISALLOW:
> +		case JOIN_OK_NO_MAP:
> +			break;
> +		case JOIN_PROTOCOL_MISMATCH:
> +			mlog(ML_NOTICE,
> +			     "This node requested DLM locking protocol %u.%u and "
> +			     "filesystem locking protocol %u.%u.  At least one of "
> +			     "the protocol versions on node %d is not compatible, "
> +			     "disconnecting\n",
> +			     dlm->dlm_locking_proto.pv_major,
> +			     dlm->dlm_locking_proto.pv_minor,
> +			     dlm->fs_locking_proto.pv_major,
> +			     dlm->fs_locking_proto.pv_minor,
> +			     node);
> +			status = -EPROTO;
> +			break;
> +		case JOIN_OK:
> +			/* Use the same locking protocol as the remote node */
> +			dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
> +			dlm->fs_locking_proto.pv_minor = packet.fs_minor;
> +			mlog(0,
> +			     "Node %d responds JOIN_OK with DLM locking protocol "
> +			     "%u.%u and fs locking protocol %u.%u\n",
> +			     node,
> +			     dlm->dlm_locking_proto.pv_major,
> +			     dlm->dlm_locking_proto.pv_minor,
> +			     dlm->fs_locking_proto.pv_major,
> +			     dlm->fs_locking_proto.pv_minor);
> +			break;
> +		default:
> +			status = -EINVAL;
> +			mlog(ML_ERROR, "invalid response %d from node %u\n",
> +			     packet.code, node);
> +			break;
> +		}
>   	}
>
>   	mlog(0, "status %d, node %d response is %d\n", status, node,
Norton.Zhu Aug. 21, 2015, 1:54 a.m. UTC | #3
Hi Srinivas,
Thanks for your advice, we should leave *response as JOIN_DISALLOW if packet.code is
invalid, I will resend the patch.

On 2015/8/21 0:56, Srinivas Eeda wrote:
> On 08/20/2015 04:50 AM, Norton.Zhu wrote:
>> Currently error handling in dlm_request_join is a little obscure.
>> So optimize it to promote readability.
>>
>> Signed-off-by: Norton.Zhu <norton.zhu@huawei.com>
>> ---
>>   fs/ocfs2/dlm/dlmdomain.c | 69 ++++++++++++++++++++++++++----------------------
>>   1 file changed, 37 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
>> index 7df88a6..af4f7aa 100644
>> --- a/fs/ocfs2/dlm/dlmdomain.c
>> +++ b/fs/ocfs2/dlm/dlmdomain.c
>> @@ -1465,39 +1465,44 @@ static int dlm_request_join(struct dlm_ctxt *dlm,
>>       if (status == -ENOPROTOOPT) {
>>           status = 0;
>>           *response = JOIN_OK_NO_MAP;
>> -    } else if (packet.code == JOIN_DISALLOW ||
>> -           packet.code == JOIN_OK_NO_MAP) {
>> -        *response = packet.code;
>> -    } else if (packet.code == JOIN_PROTOCOL_MISMATCH) {
>> -        mlog(ML_NOTICE,
>> -             "This node requested DLM locking protocol %u.%u and "
>> -             "filesystem locking protocol %u.%u.  At least one of "
>> -             "the protocol versions on node %d is not compatible, "
>> -             "disconnecting\n",
>> -             dlm->dlm_locking_proto.pv_major,
>> -             dlm->dlm_locking_proto.pv_minor,
>> -             dlm->fs_locking_proto.pv_major,
>> -             dlm->fs_locking_proto.pv_minor,
>> -             node);
>> -        status = -EPROTO;
>> -        *response = packet.code;
>> -    } else if (packet.code == JOIN_OK) {
>> -        *response = packet.code;
>> -        /* Use the same locking protocol as the remote node */
>> -        dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
>> -        dlm->fs_locking_proto.pv_minor = packet.fs_minor;
>> -        mlog(0,
>> -             "Node %d responds JOIN_OK with DLM locking protocol "
>> -             "%u.%u and fs locking protocol %u.%u\n",
>> -             node,
>> -             dlm->dlm_locking_proto.pv_major,
>> -             dlm->dlm_locking_proto.pv_minor,
>> -             dlm->fs_locking_proto.pv_major,
>> -             dlm->fs_locking_proto.pv_minor);
>>       } else {
>> -        status = -EINVAL;
>> -        mlog(ML_ERROR, "invalid response %d from node %u\n",
>> -             packet.code, node);
>> +        *response = packet.code;
> Norton, it looks much better :)
> 
> one minor comment. we don't want to reset "*response" with packet.code if it's unrecognized. We should leave the value to JOIN_DISALLOW;
> 
> rest looks good.
> 
>> +        switch (packet.code) {
>> +        case JOIN_DISALLOW:
>> +        case JOIN_OK_NO_MAP:
>> +            break;
>> +        case JOIN_PROTOCOL_MISMATCH:
>> +            mlog(ML_NOTICE,
>> +                 "This node requested DLM locking protocol %u.%u and "
>> +                 "filesystem locking protocol %u.%u.  At least one of "
>> +                 "the protocol versions on node %d is not compatible, "
>> +                 "disconnecting\n",
>> +                 dlm->dlm_locking_proto.pv_major,
>> +                 dlm->dlm_locking_proto.pv_minor,
>> +                 dlm->fs_locking_proto.pv_major,
>> +                 dlm->fs_locking_proto.pv_minor,
>> +                 node);
>> +            status = -EPROTO;
>> +            break;
>> +        case JOIN_OK:
>> +            /* Use the same locking protocol as the remote node */
>> +            dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
>> +            dlm->fs_locking_proto.pv_minor = packet.fs_minor;
>> +            mlog(0,
>> +                 "Node %d responds JOIN_OK with DLM locking protocol "
>> +                 "%u.%u and fs locking protocol %u.%u\n",
>> +                 node,
>> +                 dlm->dlm_locking_proto.pv_major,
>> +                 dlm->dlm_locking_proto.pv_minor,
>> +                 dlm->fs_locking_proto.pv_major,
>> +                 dlm->fs_locking_proto.pv_minor);
>> +            break;
>> +        default:
>> +            status = -EINVAL;
>> +            mlog(ML_ERROR, "invalid response %d from node %u\n",
>> +                 packet.code, node);
>> +            break;
>> +        }
>>       }
>>
>>       mlog(0, "status %d, node %d response is %d\n", status, node,
> 
> 
> .
>

Patch
diff mbox

diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
index 7df88a6..af4f7aa 100644
--- a/fs/ocfs2/dlm/dlmdomain.c
+++ b/fs/ocfs2/dlm/dlmdomain.c
@@ -1465,39 +1465,44 @@  static int dlm_request_join(struct dlm_ctxt *dlm,
 	if (status == -ENOPROTOOPT) {
 		status = 0;
 		*response = JOIN_OK_NO_MAP;
-	} else if (packet.code == JOIN_DISALLOW ||
-		   packet.code == JOIN_OK_NO_MAP) {
-		*response = packet.code;
-	} else if (packet.code == JOIN_PROTOCOL_MISMATCH) {
-		mlog(ML_NOTICE,
-		     "This node requested DLM locking protocol %u.%u and "
-		     "filesystem locking protocol %u.%u.  At least one of "
-		     "the protocol versions on node %d is not compatible, "
-		     "disconnecting\n",
-		     dlm->dlm_locking_proto.pv_major,
-		     dlm->dlm_locking_proto.pv_minor,
-		     dlm->fs_locking_proto.pv_major,
-		     dlm->fs_locking_proto.pv_minor,
-		     node);
-		status = -EPROTO;
-		*response = packet.code;
-	} else if (packet.code == JOIN_OK) {
-		*response = packet.code;
-		/* Use the same locking protocol as the remote node */
-		dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
-		dlm->fs_locking_proto.pv_minor = packet.fs_minor;
-		mlog(0,
-		     "Node %d responds JOIN_OK with DLM locking protocol "
-		     "%u.%u and fs locking protocol %u.%u\n",
-		     node,
-		     dlm->dlm_locking_proto.pv_major,
-		     dlm->dlm_locking_proto.pv_minor,
-		     dlm->fs_locking_proto.pv_major,
-		     dlm->fs_locking_proto.pv_minor);
 	} else {
-		status = -EINVAL;
-		mlog(ML_ERROR, "invalid response %d from node %u\n",
-		     packet.code, node);
+		*response = packet.code;
+		switch (packet.code) {
+		case JOIN_DISALLOW:
+		case JOIN_OK_NO_MAP:
+			break;
+		case JOIN_PROTOCOL_MISMATCH:
+			mlog(ML_NOTICE,
+			     "This node requested DLM locking protocol %u.%u and "
+			     "filesystem locking protocol %u.%u.  At least one of "
+			     "the protocol versions on node %d is not compatible, "
+			     "disconnecting\n",
+			     dlm->dlm_locking_proto.pv_major,
+			     dlm->dlm_locking_proto.pv_minor,
+			     dlm->fs_locking_proto.pv_major,
+			     dlm->fs_locking_proto.pv_minor,
+			     node);
+			status = -EPROTO;
+			break;
+		case JOIN_OK:
+			/* Use the same locking protocol as the remote node */
+			dlm->dlm_locking_proto.pv_minor = packet.dlm_minor;
+			dlm->fs_locking_proto.pv_minor = packet.fs_minor;
+			mlog(0,
+			     "Node %d responds JOIN_OK with DLM locking protocol "
+			     "%u.%u and fs locking protocol %u.%u\n",
+			     node,
+			     dlm->dlm_locking_proto.pv_major,
+			     dlm->dlm_locking_proto.pv_minor,
+			     dlm->fs_locking_proto.pv_major,
+			     dlm->fs_locking_proto.pv_minor);
+			break;
+		default:
+			status = -EINVAL;
+			mlog(ML_ERROR, "invalid response %d from node %u\n",
+			     packet.code, node);
+			break;
+		}
 	}

 	mlog(0, "status %d, node %d response is %d\n", status, node,