| Message ID | 566C308A.6000109@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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 <elfring@users.sourceforge.net> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> @@ -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. I can follow your concerns a bit. > 2) It puts a blank line between the allocation and the check for failure. Is there a target conflict between "convenient" variable initialisation in the declaration section and the function outline that seems to be checked by the script "checkpatch.pl" to some degree while corresponding preferences or recommendations are not mentioned in the document "CodingStyle"? > It's like a new paragraph. I do not see the separation in a strict way so far. > The allocation and the check should be next to each other. I find that these actions are still close enough in the discussed use case. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 12, 2015 at 10:49:40PM +0300, Dan Carpenter wrote: > On Sat, Dec 12, 2015 at 03:34:50PM +0100, SF Markus Elfring wrote: > > From: Markus Elfring <elfring@users.sourceforge.net> > > 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 <elfring@users.sourceforge.net> > > --- > > 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. I agree with Dan here. Please don't do it. @@ -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; param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL); if (!param) { pr_err("Unable to allocate memory for parameter.\n"); This way it would be _far_ more readable. IMHO one should have a 1 action per line of code style and only assign values in at declaration time if really necessary. But what is the benefit from this? Is it fixing a (hypothetical) bug? I somehow fail to see it. Thanks, Johannes
> @@ -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; > > param = kzalloc(sizeof(struct iscsi_param), GFP_KERNEL); > if (!param) { > pr_err("Unable to allocate memory for parameter.\n"); > > > This way it would be _far_ more readable. I guess that there are some opinions available for this implementation detail. > IMHO one should have a 1 action per line of code style How often do you care for such style issues? > and only assign values in at declaration time if really necessary. Which is or might become the official Linux coding style recommendation for this aspect? > But what is the benefit from this? Is it fixing a (hypothetical) bug? I find the shown null pointer initialisation just needless. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;