2015-10-19 09:03:31

by Tony Cho

Subject: [PATCH V3] staging: wilc1000: wilc_msgqueue.c : add goto statement

From: Leo Kim <[email protected]>

This patch uses goto statement to separet error conditionals into release_lock
and mem_free in wilc_mq_send. If unexpected errors happen, this function
cannot up the semaphore, otherwise, if no errors, the semaphore should be
released, but freeing memory is not needed in this function.

Signed-off-by: Leo Kim <[email protected]>
Signed-off-by: Tony Cho <[email protected]>
---
V3: use goto statement to seperate error types into release_lock and mem_free
---
drivers/staging/wilc1000/wilc_msgqueue.c | 25 ++++++++++++++-----------
1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c
index b13809a..bb4e93c 100644
--- a/drivers/staging/wilc1000/wilc_msgqueue.c
+++ b/drivers/staging/wilc1000/wilc_msgqueue.c
@@ -63,28 +63,31 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
if ((!pHandle) || (u32SendBufferSize == 0) || (!pvSendBuffer)) {
PRINT_ER("pHandle or pvSendBuffer is null\n");
result = -EFAULT;
- goto ERRORHANDLER;
+ goto out;
}

if (pHandle->bExiting) {
PRINT_ER("pHandle fail\n");
result = -EFAULT;
- goto ERRORHANDLER;
+ goto out;
}

spin_lock_irqsave(&pHandle->strCriticalSection, flags);

/* construct a new message */
pstrMessage = kmalloc(sizeof(Message), GFP_ATOMIC);
- if (!pstrMessage)
- return -ENOMEM;
+ if (!pstrMessage) {
+ result = -ENOMEM;
+ goto release_lock;
+ }
+
pstrMessage->u32Length = u32SendBufferSize;
pstrMessage->pstrNext = NULL;
pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize,
GFP_ATOMIC);
if (!pstrMessage->pvBuffer) {
result = -ENOMEM;
- goto ERRORHANDLER;
+ goto mem_free;
}

/* add it to the message queue */
@@ -103,13 +106,13 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,

up(&pHandle->hSem);

-ERRORHANDLER:
- /* error occured, free any allocations */
- if (pstrMessage) {
- kfree(pstrMessage->pvBuffer);
- kfree(pstrMessage);
- }
+ return 0;

+mem_free:
+ kfree(pstrMessage);
+release_lock:
+ spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
+out:
return result;
}

--
1.9.1

2015-10-19 11:42:36

by Mike Rapoport

Subject: Re: [PATCH V3] staging: wilc1000: wilc_msgqueue.c : add goto statement

On Mon, Oct 19, 2015 at 06:03:15PM +0900, Tony Cho wrote:
> From: Leo Kim <[email protected]>
>
> This patch uses goto statement to separet error conditionals into release_lock
> and mem_free in wilc_mq_send. If unexpected errors happen, this function
> cannot up the semaphore, otherwise, if no errors, the semaphore should be
> released, but freeing memory is not needed in this function.
>
> Signed-off-by: Leo Kim <[email protected]>
> Signed-off-by: Tony Cho <[email protected]>
> ---
> V3: use goto statement to seperate error types into release_lock and mem_free
> ---
> drivers/staging/wilc1000/wilc_msgqueue.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/wilc1000/wilc_msgqueue.c b/drivers/staging/wilc1000/wilc_msgqueue.c
> index b13809a..bb4e93c 100644
> --- a/drivers/staging/wilc1000/wilc_msgqueue.c
> +++ b/drivers/staging/wilc1000/wilc_msgqueue.c
> @@ -63,28 +63,31 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
> if ((!pHandle) || (u32SendBufferSize == 0) || (!pvSendBuffer)) {
> PRINT_ER("pHandle or pvSendBuffer is null\n");
> result = -EFAULT;
> - goto ERRORHANDLER;
> + goto out;

Here you can return -EFAULT beacuse no cleanup is required

> }
>
> if (pHandle->bExiting) {
> PRINT_ER("pHandle fail\n");
> result = -EFAULT;
> - goto ERRORHANDLER;
> + goto out;
> }

Ditto

> spin_lock_irqsave(&pHandle->strCriticalSection, flags);
>
> /* construct a new message */
> pstrMessage = kmalloc(sizeof(Message), GFP_ATOMIC);
> - if (!pstrMessage)
> - return -ENOMEM;
> + if (!pstrMessage) {
> + result = -ENOMEM;
> + goto release_lock;
> + }
> +

You can move the allocation outside the lock, then if the allocation
fails you can directly return -ENOMEM.


> pstrMessage->u32Length = u32SendBufferSize;
> pstrMessage->pstrNext = NULL;
> pstrMessage->pvBuffer = kmemdup(pvSendBuffer, u32SendBufferSize,
> GFP_ATOMIC);
> if (!pstrMessage->pvBuffer) {
> result = -ENOMEM;
> - goto ERRORHANDLER;
> + goto mem_free;
> }
>
> /* add it to the message queue */
> @@ -103,13 +106,13 @@ int wilc_mq_send(WILC_MsgQueueHandle *pHandle,
>
> up(&pHandle->hSem);
>
> -ERRORHANDLER:
> - /* error occured, free any allocations */
> - if (pstrMessage) {
> - kfree(pstrMessage->pvBuffer);
> - kfree(pstrMessage);
> - }
> + return 0;
>
> +mem_free:
> + kfree(pstrMessage);
> +release_lock:
> + spin_unlock_irqrestore(&pHandle->strCriticalSection, flags);
> +out:
> return result;
> }
>
> --
> 1.9.1

--
Sincerely yours,
Mike.