linux-kernel - Re: [PATCH 1/4] VMCI: Remove redundant error check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 17 May 2014 22:54:11 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Peter Senna Tschudin <peter.senna@...il.com>
Cc:	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/4] VMCI: Remove redundant error check

I like this.  There is way too much code where it sets a variable and
then checks it again a couple lines down instead of just handling it
immediately so that every line is clear without having to keep track
so many variables.

But the automated patch doesn't really go far enough in cleaning up this
function.

On Sat, May 17, 2014 at 08:22:57PM +0200, Peter Senna Tschudin wrote:
>  drivers/misc/vmw_vmci/vmci_context.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_context.c b/drivers/misc/vmw_vmci/vmci_context.c
> index f866a4b..3995e64 100644
> --- a/drivers/misc/vmw_vmci/vmci_context.c
> +++ b/drivers/misc/vmw_vmci/vmci_context.c
> @@ -836,12 +836,12 @@ int vmci_ctx_set_chkpt_state(u32 context_id,
>  	for (i = 0; i < num_ids && result == VMCI_SUCCESS; i++) {

This for loop is nonsense.  Probably it was left over cruft.  It should
be:

	for (i = 0; i < num_ids; i++) {

>  		current_id = ((u32 *)cpt_buf)[i];
>  		result = vmci_ctx_add_notification(context_id, current_id);
> -		if (result != VMCI_SUCCESS)
> +		if (result != VMCI_SUCCESS) {
> +			pr_devel("Failed to set cpt state (type=%d) (error=%d)\n",
> +				 cpt_type, result);
>  			break;

			return result;

Why break when it's just going to return?  Eliminate the need to track
this error.

> +		}
>  	}
> -	if (result != VMCI_SUCCESS)
> -		pr_devel("Failed to set cpt state (type=%d) (error=%d)\n",
> -			 cpt_type, result);
>  
>  	return result;

	return VMCI_SUCCESS;

Then we can remove the initialization for "result" as well to eliminate
another forced look up to the start of the function and down.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/