[KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation examples

[KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation examples

[Thomas Hisch]

Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
---
 Documentation/connector/cn_test.c                  |    4 +---
 .../filesystems/configfs/configfs_example.c        |    8 ++------
 .../firmware_sample_firmware_class.c               |    6 ++----
 3 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/Documentation/connector/cn_test.c b/Documentation/connector/cn_test.c
index 3e73231..c19fc86 100644
--- a/Documentation/connector/cn_test.c
+++ b/Documentation/connector/cn_test.c
@@ -124,10 +124,8 @@ static void cn_test_timer_func(unsigned long __data)
 	struct cn_msg *m;
 	char data[32];
 
-	m = kmalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
+	m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
 	if (m) {
-		memset(m, 0, sizeof(*m) + sizeof(data));
-
 		memcpy(&m->id, &cn_test_id, sizeof(m->id));
 		m->seq = cn_test_timer_counter;
 		m->len = sizeof(data);
diff --git a/Documentation/filesystems/configfs/configfs_example.c b/Documentation/filesystems/configfs/configfs_example.c
index 2d6a14a..473433c 100644
--- a/Documentation/filesystems/configfs/configfs_example.c
+++ b/Documentation/filesystems/configfs/configfs_example.c
@@ -277,12 +277,10 @@ static struct config_item *simple_children_make_item(struct config_group *group,
 {
 	struct simple_child *simple_child;
 
-	simple_child = kmalloc(sizeof(struct simple_child), GFP_KERNEL);
+	simple_child = kzalloc(sizeof(struct simple_child), GFP_KERNEL);
 	if (!simple_child)
 		return NULL;
 
-	memset(simple_child, 0, sizeof(struct simple_child));
-
 	config_item_init_type_name(&simple_child->item, name,
 				   &simple_child_type);
 
@@ -364,13 +362,11 @@ static struct config_group *group_children_make_group(struct config_group *group
 {
 	struct simple_children *simple_children;
 
-	simple_children = kmalloc(sizeof(struct simple_children),
+	simple_children = kzalloc(sizeof(struct simple_children),
 				  GFP_KERNEL);
 	if (!simple_children)
 		return NULL;
 
-	memset(simple_children, 0, sizeof(struct simple_children));
-
 	config_group_init_type_name(&simple_children->group, name,
 				    &simple_children_type);
 
diff --git a/Documentation/firmware_class/firmware_sample_firmware_class.c b/Documentation/firmware_class/firmware_sample_firmware_class.c
index 9e1b0e4..da888f0 100644
--- a/Documentation/firmware_class/firmware_sample_firmware_class.c
+++ b/Documentation/firmware_class/firmware_sample_firmware_class.c
@@ -108,15 +108,13 @@ static int fw_setup_class_device(struct class_device *class_dev,
 				 struct device *device)
 {
 	int retval = 0;
-	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
+	struct firmware_priv *fw_priv = kzalloc(sizeof(struct firmware_priv),
 						GFP_KERNEL);
 
 	if(!fw_priv){
 		retval = -ENOMEM;
 		goto out;
 	}
-	memset(fw_priv, 0, sizeof(*fw_priv));
-	memset(class_dev, 0, sizeof(*class_dev));
 
 	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
 	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
@@ -180,7 +178,7 @@ static int __init firmware_sample_init(void)
 	int error;
 
 	device_initialize(&my_device);
-	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
+	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
 	if(!class_dev)
 		return -ENOMEM;
 
-- 
1.5.0.rc0.ge900

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Robert P. J. Day]

On Tue, 2 Jan 2007, Thomas Hisch wrote:

> Signed-off-by: Thomas Hisch <t.hisch@gmail.com>
> ---
>  Documentation/connector/cn_test.c                  |    4 +---
>  .../filesystems/configfs/configfs_example.c        |    8 ++------
>  .../firmware_sample_firmware_class.c               |    6 ++----
>  3 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/connector/cn_test.c b/Documentation/connector/cn_test.c
> index 3e73231..c19fc86 100644
> --- a/Documentation/connector/cn_test.c
> +++ b/Documentation/connector/cn_test.c
> @@ -124,10 +124,8 @@ static void cn_test_timer_func(unsigned long __data)
>  	struct cn_msg *m;
>  	char data[32];
>
> -	m = kmalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> +	m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
>  	if (m) {
> -		memset(m, 0, sizeof(*m) + sizeof(data));
> -
>  		memcpy(&m->id, &cn_test_id, sizeof(m->id));
>  		m->seq = cn_test_timer_counter;
>  		m->len = sizeof(data);
> diff --git a/Documentation/filesystems/configfs/configfs_example.c b/Documentation/filesystems/configfs/configfs_example.c
> index 2d6a14a..473433c 100644
> --- a/Documentation/filesystems/configfs/configfs_example.c
> +++ b/Documentation/filesystems/configfs/configfs_example.c
> @@ -277,12 +277,10 @@ static struct config_item *simple_children_make_item(struct config_group *group,
>  {
>  	struct simple_child *simple_child;
>
> -	simple_child = kmalloc(sizeof(struct simple_child), GFP_KERNEL);
> +	simple_child = kzalloc(sizeof(struct simple_child), GFP_KERNEL);

while you're making changes like this, you should also change the
sizeof() usage to reflect what's recommended in the CodingStyle
document:

	simple_child = kzalloc(sizeof(*simple_child), GFP_KERNEL);

rday
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Robert P. J. Day]

On Tue, 2 Jan 2007, Thomas Hisch wrote:

> diff --git a/Documentation/firmware_class/firmware_sample_firmware_class.c b/Documentation/firmware_class/firmware_sample_firmware_class.c
> index 9e1b0e4..da888f0 100644
> --- a/Documentation/firmware_class/firmware_sample_firmware_class.c
> +++ b/Documentation/firmware_class/firmware_sample_firmware_class.c
> @@ -108,15 +108,13 @@ static int fw_setup_class_device(struct class_device *class_dev,
>  				 struct device *device)
>  {
>  	int retval = 0;
> -	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
> +	struct firmware_priv *fw_priv = kzalloc(sizeof(struct firmware_priv),
>  						GFP_KERNEL);
>
>  	if(!fw_priv){
>  		retval = -ENOMEM;
>  		goto out;
>  	}
> -	memset(fw_priv, 0, sizeof(*fw_priv));
> -	memset(class_dev, 0, sizeof(*class_dev));

i don't think you can remove this second memset() call.  it represents
an argument to this routine, and there's nothing about that kzalloc()
call that lets you skip that step.  (that first memset() call is, of
course, fine.)

>  	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
>  	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
> @@ -180,7 +178,7 @@ static int __init firmware_sample_init(void)
>  	int error;
>
>  	device_initialize(&my_device);
> -	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
> +	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
>  	if(!class_dev)
>  		return -ENOMEM;

this also seems inappropriate.  the only kmalloc() calls that should
be converted to kzalloc() calls are ones for which the allocated
memory is set immediately to zero.  that's not what's happening here,
so you should leave the original kmalloc() call as it is.

rday
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Thomas Hisch]

On Tue, Jan 02, 2007 at 08:40:26AM -0500, Robert P. J. Day wrote:
> On Tue, 2 Jan 2007, Thomas Hisch wrote:
> 
> > diff --git a/Documentation/firmware_class/firmware_sample_firmware_class.c b/Documentation/firmware_class/firmware_sample_firmware_class.c
> > index 9e1b0e4..da888f0 100644
> > --- a/Documentation/firmware_class/firmware_sample_firmware_class.c
> > +++ b/Documentation/firmware_class/firmware_sample_firmware_class.c
> > @@ -108,15 +108,13 @@ static int fw_setup_class_device(struct class_device *class_dev,
> >  				 struct device *device)
> >  {
> >  	int retval = 0;
> > -	struct firmware_priv *fw_priv = kmalloc(sizeof(struct firmware_priv),
> > +	struct firmware_priv *fw_priv = kzalloc(sizeof(struct firmware_priv),
> >  						GFP_KERNEL);
> >
> >  	if(!fw_priv){
> >  		retval = -ENOMEM;
> >  		goto out;
> >  	}
> > -	memset(fw_priv, 0, sizeof(*fw_priv));
> > -	memset(class_dev, 0, sizeof(*class_dev));
> 
> i don't think you can remove this second memset() call.  it represents
> an argument to this routine, and there's nothing about that kzalloc()
> call that lets you skip that step.  (that first memset() call is, of
> course, fine.)
> 
> >  	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
> >  	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
> > @@ -180,7 +178,7 @@ static int __init firmware_sample_init(void)
> >  	int error;
> >
> >  	device_initialize(&my_device);
> > -	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
> > +	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
> >  	if(!class_dev)
> >  		return -ENOMEM;
> 
> this also seems inappropriate.  the only kmalloc() calls that should
> be converted to kzalloc() calls are ones for which the allocated
> memory is set immediately to zero.  that's not what's happening here,
> so you should leave the original kmalloc() call as it is.
> 

what is the drawback of changing above kmalloc calls to kzalloc?

nevertheless i'll will drop this inappropriate kzalloc part from the patch. 

-- 
Thomas Hisch
e0625874@stud.tuwien.ac.at
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Robert P. J. Day]

On Tue, 2 Jan 2007, Thomas Hisch wrote:

> On Tue, Jan 02, 2007 at 08:40:26AM -0500, Robert P. J. Day wrote:

> > >  	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
> > >  	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
> > > @@ -180,7 +178,7 @@ static int __init firmware_sample_init(void)
> > >  	int error;
> > >
> > >  	device_initialize(&my_device);
> > > -	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
> > > +	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
> > >  	if(!class_dev)
> > >  		return -ENOMEM;
> >
> > this also seems inappropriate.  the only kmalloc() calls that
> > should be converted to kzalloc() calls are ones for which the
> > allocated memory is set immediately to zero.  that's not what's
> > happening here, so you should leave the original kmalloc() call as
> > it is.
>
> what is the drawback of changing above kmalloc calls to kzalloc?

because you would be changing the actual logic of the code.  a
kmalloc() followed by a memset() to zero can reasonably be replaced by
a single kzalloc() call since it effectively doesn't change the code
logic.

but replacing a kmalloc() by a kzalloc() definitely changes the
underlying processing.  it *might* not cause any problem whatsoever,
but that's not the point.  it simply doesn't represent the same code
anymore, that's all.  not better, not worse -- just different.

rday
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Arnaldo Carvalho de Melo]

On Tue, Jan 02, 2007 at 10:39:01AM -0500, Robert P. J. Day wrote:
> On Tue, 2 Jan 2007, Thomas Hisch wrote:
> 
> > On Tue, Jan 02, 2007 at 08:40:26AM -0500, Robert P. J. Day wrote:
> 
> > > >  	strncpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
> > > >  	fw_priv->fw_id[FIRMWARE_NAME_MAX-1] = '\0';
> > > > @@ -180,7 +178,7 @@ static int __init firmware_sample_init(void)
> > > >  	int error;
> > > >
> > > >  	device_initialize(&my_device);
> > > > -	class_dev = kmalloc(sizeof(struct class_device), GFP_KERNEL);
> > > > +	class_dev = kzalloc(sizeof(struct class_device), GFP_KERNEL);
> > > >  	if(!class_dev)
> > > >  		return -ENOMEM;
> > >
> > > this also seems inappropriate.  the only kmalloc() calls that
> > > should be converted to kzalloc() calls are ones for which the
> > > allocated memory is set immediately to zero.  that's not what's
> > > happening here, so you should leave the original kmalloc() call as
> > > it is.
> >
> > what is the drawback of changing above kmalloc calls to kzalloc?
> 
> because you would be changing the actual logic of the code.  a
> kmalloc() followed by a memset() to zero can reasonably be replaced by
> a single kzalloc() call since it effectively doesn't change the code
> logic.
> 
> but replacing a kmalloc() by a kzalloc() definitely changes the
> underlying processing.  it *might* not cause any problem whatsoever,
> but that's not the point.  it simply doesn't represent the same code
> anymore, that's all.  not better, not worse -- just different.

Worse, as it consumes more processor cycles, if one is allocating a
struct and initializing explicitely all its fields or just don't care
about what is the initial values that comes in the area kmalloc'ed the
memset is just that, contributing to global warming 8-)

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Alexey Dobriyan]

On Tue, Jan 02, 2007 at 08:34:44AM -0500, Robert P. J. Day wrote:
> On Tue, 2 Jan 2007, Thomas Hisch wrote:
> > --- a/Documentation/connector/cn_test.c
> > +++ b/Documentation/connector/cn_test.c
> > @@ -124,10 +124,8 @@ static void cn_test_timer_func(unsigned long __data)
> >  	struct cn_msg *m;
> >  	char data[32];
> >
> > -	m = kmalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> > +	m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> >  	if (m) {
> > -		memset(m, 0, sizeof(*m) + sizeof(data));
> > -
> >  		memcpy(&m->id, &cn_test_id, sizeof(m->id));
> >  		m->seq = cn_test_timer_counter;
> >  		m->len = sizeof(data);
> > diff --git a/Documentation/filesystems/configfs/configfs_example.c b/Documentation/filesystems/configfs/configfs_example.c
> > index 2d6a14a..473433c 100644
> > --- a/Documentation/filesystems/configfs/configfs_example.c
> > +++ b/Documentation/filesystems/configfs/configfs_example.c
> > @@ -277,12 +277,10 @@ static struct config_item *simple_children_make_item(struct config_group *group,
> >  {
> >  	struct simple_child *simple_child;
> >
> > -	simple_child = kmalloc(sizeof(struct simple_child), GFP_KERNEL);
> > +	simple_child = kzalloc(sizeof(struct simple_child), GFP_KERNEL);
>
> while you're making changes like this, you should also change the
> sizeof() usage to reflect what's recommended in the CodingStyle
> document:
>
> 	simple_child = kzalloc(sizeof(*simple_child), GFP_KERNEL);

No, you shouldn't! This particular coding style generated a long thread
on l-k with folks being pro and against.

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[Robert P. J. Day]

On Fri, 5 Jan 2007, tom hisch wrote:

> On 1/5/07, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Tue, Jan 02, 2007 at 08:34:44AM -0500, Robert P. J. Day wrote:
> > > On Tue, 2 Jan 2007, Thomas Hisch wrote:
> > > > --- a/Documentation/connector/cn_test.c
> > > > +++ b/Documentation/connector/cn_test.c
> > > > @@ -124,10 +124,8 @@ static void cn_test_timer_func(unsigned long
> > __data)
> > > >     struct cn_msg *m;
> > > >     char data[32];
> > > >
> > > > -   m = kmalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> > > > +   m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> > > >     if (m) {
> > > > -           memset(m, 0, sizeof(*m) + sizeof(data));
> > > > -
> > > >             memcpy(&m->id, &cn_test_id, sizeof(m->id));
> > > >             m->seq = cn_test_timer_counter;
> > > >             m->len = sizeof(data);
> > > > diff --git a/Documentation/filesystems/configfs/configfs_example.c
> > b/Documentation/filesystems/configfs/configfs_example.c
> > > > index 2d6a14a..473433c 100644
> > > > --- a/Documentation/filesystems/configfs/configfs_example.c
> > > > +++ b/Documentation/filesystems/configfs/configfs_example.c
> > > > @@ -277,12 +277,10 @@ static struct config_item
> > *simple_children_make_item(struct config_group *group,
> > > >  {
> > > >     struct simple_child *simple_child;
> > > >
> > > > -   simple_child = kmalloc(sizeof(struct simple_child), GFP_KERNEL);
> > > > +   simple_child = kzalloc(sizeof(struct simple_child), GFP_KERNEL);
> > >
> > > while you're making changes like this, you should also change the
> > > sizeof() usage to reflect what's recommended in the CodingStyle
> > > document:
> > >
> > >       simple_child = kzalloc(sizeof(*simple_child), GFP_KERNEL);
> >
> > No, you shouldn't! This particular coding style generated a long thread
> > on l-k with folks being pro and against.
>
> ok. but this should be mentioned in the CodingStyle document, to avoid
> other patches having the same sizeof* coding style.

i don't think you're ever going to get universal agreement on this.  a
while back, i was told (in no uncertain terms) that if i *didn't* add
that cleanup to a patch i was submitting, that patch would be dropped.

and today, it appears that, if that cleanup *is* added, the subsequent
patch will be rejected in some circumstances.

go figure.

rday
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] kmalloc -> kzalloc conversion in Documentation

[tom hisch]

On 1/5/07, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Tue, Jan 02, 2007 at 08:34:44AM -0500, Robert P. J. Day wrote:
> > On Tue, 2 Jan 2007, Thomas Hisch wrote:
> > > --- a/Documentation/connector/cn_test.c
> > > +++ b/Documentation/connector/cn_test.c
> > > @@ -124,10 +124,8 @@ static void cn_test_timer_func(unsigned long __data)
> > >     struct cn_msg *m;
> > >     char data[32];
> > >
> > > -   m = kmalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> > > +   m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);
> > >     if (m) {
> > > -           memset(m, 0, sizeof(*m) + sizeof(data));
> > > -
> > >             memcpy(&m->id, &cn_test_id, sizeof(m->id));
> > >             m->seq = cn_test_timer_counter;
> > >             m->len = sizeof(data);
> > > diff --git a/Documentation/filesystems/configfs/configfs_example.c b/Documentation/filesystems/configfs/configfs_example.c
> > > index 2d6a14a..473433c 100644
> > > --- a/Documentation/filesystems/configfs/configfs_example.c
> > > +++ b/Documentation/filesystems/configfs/configfs_example.c
> > > @@ -277,12 +277,10 @@ static struct config_item *simple_children_make_item(struct config_group *group,
> > >  {
> > >     struct simple_child *simple_child;
> > >
> > > -   simple_child = kmalloc(sizeof(struct simple_child), GFP_KERNEL);
> > > +   simple_child = kzalloc(sizeof(struct simple_child), GFP_KERNEL);
> >
> > while you're making changes like this, you should also change the
> > sizeof() usage to reflect what's recommended in the CodingStyle
> > document:
> >
> >       simple_child = kzalloc(sizeof(*simple_child), GFP_KERNEL);
>
> No, you shouldn't! This particular coding style generated a long thread
> on l-k with folks being pro and against.
>
>

ok. but this should be mentioned in the CodingStyle document, to avoid
other patches having the same sizeof* coding style.

regards
thomas hisch
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors