Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1639851 > unrolled thread

[PATCH 2/4] vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/4] vmbus: Delete an error message for a failed memory  allocation in vmbus_device_create() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-11 18:20 +0200
    Re: vmbus: Delete an error message for a failed memory allocation in  vmbus_device_create() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-11 18:40 +0200
      Re: vmbus: Delete an error message for a failed memory allocation  in vmbus_device_create() Stephen Hemminger <stephen@networkplumber.org> - 2017-05-11 18:50 +0200
    Re: [PATCH 2/4] vmbus: Delete an error message for a failed memory  allocation in vmbus_device_create() Stephen Hemminger <stephen@networkplumber.org> - 2017-05-11 18:40 +0200
      Re: [PATCH 2/4] vmbus: Delete an error message for a failed memory  allocation in vmbus_device_create() Dan Carpenter <dan.carpenter@oracle.com> - 2017-05-12 09:20 +0200
        Re: vmbus: Delete an error message for a failed memory allocation in  vmbus_device_create() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-12 09:40 +0200
          Re: vmbus: Delete an error message for a failed memory allocation  in vmbus_device_create() Joe Perches <joe@perches.com> - 2017-05-12 10:00 +0200
            Re: Clarification for general change acceptance SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-12 10:30 +0200
              Re: Clarification for general change acceptance Julia Lawall <julia.lawall@lip6.fr> - 2017-05-12 10:40 +0200
                Re: Clarification for general change acceptance SF Markus Elfring <elfring@users.sourceforge.net> - 2017-05-12 10:50 +0200

#1639851 — [PATCH 2/4] vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 11 May 2017 17:33:14 +0200

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hv/vmbus_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 6802d74f162c..96328aebae5a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1149,7 +1149,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
-	if (!child_device_obj) {
-		pr_err("Unable to allocate device object for child device\n");
+	if (!child_device_obj)
 		return NULL;
-	}
 
 	child_device_obj->channel = channel;
 	memcpy(&child_device_obj->dev_type, type, sizeof(uuid_le));
-- 
2.12.3

[toc] | [next] | [standalone]

#1639890 — Re: vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

> Taking out the message assumes that all callers of this function either log an
> error or pass appropriate error code back to userspace.

Do you like the default error response by Linux memory allocation functions?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1639897 — Re: vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

On Thu, 11 May 2017 18:36:44 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Taking out the message assumes that all callers of this function either log an
> > error or pass appropriate error code back to userspace.  
> 
> Do you like the default error response by Linux memory allocation functions?

The default error message only helps a little.
I doubt this will ever fail anyway since only allocated on boot.

[toc] | [prev] | [next] | [standalone]

#1639891

On Thu, 11 May 2017 18:17:01 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 11 May 2017 17:33:14 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/hv/vmbus_drv.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 6802d74f162c..96328aebae5a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1149,7 +1149,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
> -	if (!child_device_obj) {
> -		pr_err("Unable to allocate device object for child device\n");
> +	if (!child_device_obj)
>  		return NULL;
> -	}
>  
>  	child_device_obj->channel = channel;
>  	memcpy(&child_device_obj->dev_type, type, sizeof(uuid_le));

Taking out the message assumes that all callers of this function either log an
error or pass appropriate error code back to userspace. Did you walk back
through all the callers?

Just because an automated tool says that this needs to change does not
mean it has to.

[toc] | [prev] | [next] | [standalone]

#1640243

On Thu, May 11, 2017 at 09:30:15AM -0700, Stephen Hemminger wrote:
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 6802d74f162c..96328aebae5a 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1149,7 +1149,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
> > -	if (!child_device_obj) {
> > -		pr_err("Unable to allocate device object for child device\n");
> > +	if (!child_device_obj)
> >  		return NULL;
> > -	}
> >  
> >  	child_device_obj->channel = channel;
> >  	memcpy(&child_device_obj->dev_type, type, sizeof(uuid_le));
> 
> Taking out the message assumes that all callers of this function either log an
> error or pass appropriate error code back to userspace. Did you walk back
> through all the callers?
> 
> Just because an automated tool says that this needs to change does not
> mean it has to.

Checkpatch.pl is correct here.  This message is useless.  It's during
init so it's unlikely to fail ever.  In current kernels small kmallocs
are quaranteed to succeed so it can't actually fail currently.  The
stack trace is more useful than this message because it tells you a lot
about what memory is free and the whole call tree.

The error message is dead useless code.

This patch is not going to be merged because Markus doesn't listen to
feedback and he's blocked but otherwise it's an OK patch.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1640256 — Re: vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

>> Just because an automated tool says that this needs to change does not
>> mean it has to.
> 
> Checkpatch.pl is correct here.  This message is useless.  It's during
> init so it's unlikely to fail ever.  In current kernels small kmallocs
> are quaranteed to succeed so it can't actually fail currently.  The
> stack trace is more useful than this message because it tells you a lot
> about what memory is free and the whole call tree.
> 
> The error message is dead useless code.

Would you like to clarify corresponding software evolution any more?

Is there a need for better documentation of the involved programming interfaces?


> This patch is not going to be merged because Markus doesn't listen to
> feedback and he's blocked but otherwise it's an OK patch.

Does this information contain a contradiction?
Will patches be picked up also from contributors who got a special
development reputation anyhow?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1640275 — Re: vmbus: Delete an error message for a failed memory allocation in vmbus_device_create()

On Fri, 2017-05-12 at 09:32 +0200, SF Markus Elfring wrote:
> Will patches be picked up also from contributors who got a special
> development reputation anyhow?

Yes.

Developer reputation matters for somewhat controversial
patches being applied as well as non-controversial and
obviously correct patches being ignored.

Your reputation means most all of your patches fall into
the latter category.

Look up "wheat and chaff".

Your patches are mostly chaff.

You have produced many trivial patches that have caused
new defects.  That is simply unacceptable.  Especially
when you don't immediately fix the problems you cause.

If you would stop producing the trivial and instead
channel your efforts into actual bug fixing and logic
corrections and not just style modifications with no
code impact, your patch acceptance rate would increase.

Yes, your daily patch production rate would fall.
Likely every reader of LKML but you would be happy.

I have given you many suggestions for actual structural
improvements to kernel code.

You have ignored _all_ of them and I am unlikely to try
to interact with you any longer until your wheat:chaff
ratio changes.

[toc] | [prev] | [next] | [standalone]

#1640310 — Re: Clarification for general change acceptance

> Developer reputation matters for somewhat controversial
> patches being applied as well as non-controversial and
> obviously correct patches being ignored.

I am aware that there are more factors involved.


> Your reputation means most all of your patches fall into
> the latter category.

I hope that this situation will evolve into directions which you would prefer more.


> You have produced many trivial patches

This is true.

I started my concrete contributions to Linux software modules with simple
source code search patterns.


> that have caused new defects.

A few unwanted programming mistakes just happened somehow.


> That is simply unacceptable.

Glitches are not desired as usual.



> Especially when you don't immediately fix the problems you cause.

I find my response times reasonable to some degree so far.

Remaining open issues can be clarified by a corresponding constructive
development dialogue, can't they?



> If you would stop producing the trivial and instead
> channel your efforts into actual bug fixing and logic
> corrections and not just style modifications with no
> code impact, your patch acceptance rate would increase.

I find your conclusion appropriate.

But I will come along source code places where I am going to update details
which are also trivial.


> I have given you many suggestions for actual structural
> improvements to kernel code.

I have got an other impression. There were a few occasions where advanced
change possibilities were proposed.


> You have ignored _all_ of them and I am unlikely to try
> to interact with you any longer until your wheat:chaff
> ratio changes.

Can the efforts for deleting questionable error messages around Linux memory
allocation functions improve this situation?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1640315 — Re: Clarification for general change acceptance

> But I will come along source code places where I am going to update details
> which are also trivial.

When you make a patch, you are not obliged to eliminate all of the other
checkpatch warnings on the file.  I don't know where you got this idea
from.  The submitting patch guidelines don't say to do it, and no one else
does it.  You just need to ensure that your changes don't introduce new
warnings, unless there is a good reason to do so.

julia

[toc] | [prev] | [next] | [standalone]

#1640323 — Re: Clarification for general change acceptance

> When you make a patch, you are not obliged to eliminate all of the other
> checkpatch warnings on the file.

Your view is generally fine.


> I don't know where you got this idea from.

I got used as a professional software developer to some approaches for
reducing development warnings to some degree. So I picked further update
suggestions up also from this source code analysis tool.

Regards,
Markus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web