From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752482AbbLLTt7 (ORCPT ); Sat, 12 Dec 2015 14:49:59 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:47623 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750940AbbLLTt5 (ORCPT ); Sat, 12 Dec 2015 14:49:57 -0500 Date: Sat, 12 Dec 2015 22:49:40 +0300 From: Dan Carpenter To: SF Markus Elfring Cc: linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, "Nicholas A. Bellinger" , LKML , kernel-janitors@vger.kernel.org, Julia Lawall Subject: Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly Message-ID: <20151212194939.GQ5284@mwanda> References: <566ABCD9.1060404@users.sourceforge.net> <566C2F7B.6030704@users.sourceforge.net> <566C308A.6000109@users.sourceforge.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <566C308A.6000109@users.sourceforge.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sat, 12 Dec 2015 11:36:02 +0100 > > Omit the unnecessary setting to a null pointer for the variable "param" > at the beginning of the function "iscsi_set_default_param" > because it can be directly initialized with the return value > from the function "kzalloc". > > Signed-off-by: Markus Elfring > --- > drivers/target/iscsi/iscsi_target_parameters.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c > index 3a1f9a7..0a8bd3f 100644 > --- a/drivers/target/iscsi/iscsi_target_parameters.c > +++ b/drivers/target/iscsi/iscsi_target_parameters.c > @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para > char *name, char *value, u8 phase, u8 scope, u8 sender, > u16 type_range, u8 use) > { > - struct iscsi_param *param = NULL; > + struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL); > > - param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL); > if (!param) { > pr_err("Unable to allocate memory for parameter.\n"); > goto out; It's better to just get rid of the initialization but leave the kzalloc() as-is for two reasons. 1) Initializer code normally contains more bugs per line than other code. I am thinking about dereferencing pointers before checking for NULL or not checking the allocation for failure. 2) It puts a blank line between the allocation and the check for failure. It's like a new paragraph. The allocation and the check should be next to each other. regards, dan carpenter From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Carpenter Date: Sat, 12 Dec 2015 19:49:40 +0000 Subject: Re: [PATCH 1/7] iscsi-target: Use a variable initialisation in iscsi_set_default_param() directly Message-Id: <20151212194939.GQ5284@mwanda> List-Id: References: <566ABCD9.1060404@users.sourceforge.net> <566C2F7B.6030704@users.sourceforge.net> <566C308A.6000109@users.sourceforge.net> In-Reply-To: <566C308A.6000109@users.sourceforge.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: SF Markus Elfring Cc: linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, "Nicholas A. Bellinger" , LKML , kernel-janitors@vger.kernel.org, Julia Lawall On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sat, 12 Dec 2015 11:36:02 +0100 > > Omit the unnecessary setting to a null pointer for the variable "param" > at the beginning of the function "iscsi_set_default_param" > because it can be directly initialized with the return value > from the function "kzalloc". > > Signed-off-by: Markus Elfring > --- > drivers/target/iscsi/iscsi_target_parameters.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_parameters.c b/drivers/target/iscsi/iscsi_target_parameters.c > index 3a1f9a7..0a8bd3f 100644 > --- a/drivers/target/iscsi/iscsi_target_parameters.c > +++ b/drivers/target/iscsi/iscsi_target_parameters.c > @@ -127,9 +127,8 @@ static struct iscsi_param *iscsi_set_default_param(struct iscsi_param_list *para > char *name, char *value, u8 phase, u8 scope, u8 sender, > u16 type_range, u8 use) > { > - struct iscsi_param *param = NULL; > + struct iscsi_param *param = kzalloc(sizeof(*param), GFP_KERNEL); > > - param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL); > if (!param) { > pr_err("Unable to allocate memory for parameter.\n"); > goto out; It's better to just get rid of the initialization but leave the kzalloc() as-is for two reasons. 1) Initializer code normally contains more bugs per line than other code. I am thinking about dereferencing pointers before checking for NULL or not checking the allocation for failure. 2) It puts a blank line between the allocation and the check for failure. It's like a new paragraph. The allocation and the check should be next to each other. regards, dan carpenter