[PATCH 0/4] VMBus: Adjustments for some function implementations

[PATCH 0/4] VMBus: Adjustments for some function implementations

[SF Markus Elfring]

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

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  Improve a size determination in vmbus_device_create()
  Delete an error message for a failed memory allocation in vmbus_device_create()
  Fix a typo in a comment line
  Adjust five checks for null pointers

 drivers/hv/vmbus_drv.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

-- 
2.12.3

[PATCH 1/4] vmbus: Improve a size determination in vmbus_device_create()

[SF Markus Elfring]

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

Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hv/vmbus_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0087b49095eb..6802d74f162c 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1145,5 +1145,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
 {
 	struct hv_device *child_device_obj;
 
-	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
+	child_device_obj = kzalloc(sizeof(*child_device_obj), GFP_KERNEL);
 	if (!child_device_obj) {
-- 
2.12.3

Re: [PATCH 1/4] vmbus: Improve a size determination in vmbus_device_create()

[Stephen Hemminger]

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

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 11 May 2017 17:30:10 +0200
> 
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/hv/vmbus_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 0087b49095eb..6802d74f162c 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1145,5 +1145,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
>  {
>  	struct hv_device *child_device_obj;
>  
> -	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
> +	child_device_obj = kzalloc(sizeof(*child_device_obj), GFP_KERNEL);
>  	if (!child_device_obj) {

This looks fine.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH 1/4] vmbus: Improve a size determination in vmbus_device_create()

[Greg KH]

On Thu, May 11, 2017 at 09:30:56AM -0700, Stephen Hemminger wrote:
> On Thu, 11 May 2017 18:15:46 +0200
> SF Markus Elfring <elfring@users.sourceforge.net> wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 11 May 2017 17:30:10 +0200
> > 
> > Replace the specification of a data structure by a pointer dereference
> > as the parameter for the operator "sizeof" to make the corresponding size
> > determination a bit safer according to the Linux coding style convention.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  drivers/hv/vmbus_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > index 0087b49095eb..6802d74f162c 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -1145,5 +1145,5 @@ struct hv_device *vmbus_device_create(const uuid_le *type,
> >  {
> >  	struct hv_device *child_device_obj;
> >  
> > -	child_device_obj = kzalloc(sizeof(struct hv_device), GFP_KERNEL);
> > +	child_device_obj = kzalloc(sizeof(*child_device_obj), GFP_KERNEL);
> >  	if (!child_device_obj) {
> 
> This looks fine.
> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Note, I have a blacklist filter for this person for a reason, I, and
many other maintainers, just ignore them for good reason...

greg k-h

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

[SF Markus Elfring]

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

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

[Stephen Hemminger]

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.

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

[SF Markus Elfring]

> 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

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

[Stephen Hemminger]

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.

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

[Dan Carpenter]

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

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

[SF Markus Elfring]

>> 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

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

[Joe Perches]

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.

Re: Clarification for general change acceptance

[SF Markus Elfring]

> 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

Re: Clarification for general change acceptance

[Julia Lawall]

> 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

Re: Clarification for general change acceptance

[SF Markus Elfring]

> 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

[PATCH 3/4] vmbus: Fix a typo in a comment line

[SF Markus Elfring]

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

Add a missing character in this description.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hv/vmbus_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 96328aebae5a..ff94b111ed8d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -991,7 +991,7 @@ static void vmbus_isr(void)
 		/*
 		 * Our host is win8 or above. The signaling mechanism
 		 * has changed and we can directly look at the event page.
-		 * If bit n is set then we have an interrup on the channel
+		 * If bit n is set then we have an interrupt on the channel
 		 * whose id is n.
 		 */
 		handled = true;
-- 
2.12.3

Re: [PATCH 3/4] vmbus: Fix a typo in a comment line

[Stephen Hemminger]

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

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 11 May 2017 17:43:55 +0200
> 
> Add a missing character in this description.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/hv/vmbus_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 96328aebae5a..ff94b111ed8d 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -991,7 +991,7 @@ static void vmbus_isr(void)
>  		/*
>  		 * Our host is win8 or above. The signaling mechanism
>  		 * has changed and we can directly look at the event page.
> -		 * If bit n is set then we have an interrup on the channel
> +		 * If bit n is set then we have an interrupt on the channel
>  		 * whose id is n.
>  		 */
>  		handled = true;


Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH 4/4] vmbus: Adjust five checks for null pointers

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 11 May 2017 17:52:38 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written …

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hv/vmbus_drv.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index ff94b111ed8d..b55b979ecf8a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -93,7 +93,7 @@ static DEFINE_SEMAPHORE(hyperv_mmio_lock);
 
 static int vmbus_exists(void)
 {
-	if (hv_acpi_dev == NULL)
+	if (!hv_acpi_dev)
 		return -ENODEV;
 
 	return 0;
@@ -568,7 +568,7 @@ static const struct hv_vmbus_device_id *hv_vmbus_get_id(struct hv_driver *drv,
 		return id;
 
 	id = drv->id_table;
-	if (id == NULL)
+	if (!id)
 		return NULL; /* empty device table */
 
 	for (; !is_null_guid(&id->guid); id++)
@@ -871,7 +871,7 @@ void vmbus_on_msg_dpc(unsigned long data)
 	entry = &channel_message_table[hdr->msgtype];
 	if (entry->handler_type	== VMHT_BLOCKING) {
 		ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
-		if (ctx == NULL)
+		if (!ctx)
 			return;
 
 		INIT_WORK(&ctx->work, vmbus_onmessage_work);
@@ -894,7 +894,7 @@ static void vmbus_channel_isr(struct vmbus_channel *channel)
 	void (*callback_fn)(void *);
 
 	callback_fn = READ_ONCE(channel->onchannel_callback);
-	if (likely(callback_fn != NULL))
+	if (likely(callback_fn))
 		(*callback_fn)(channel->channel_callback_context);
 }
 
@@ -970,7 +970,7 @@ static void vmbus_isr(void)
 	union hv_synic_event_flags *event;
 	bool handled = false;
 
-	if (unlikely(page_addr == NULL))
+	if (unlikely(!page_addr))
 		return;
 
 	event = (union hv_synic_event_flags *)page_addr +
-- 
2.12.3

Re: [PATCH 4/4] vmbus: Adjust five checks for null pointers

[Stephen Hemminger]

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

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 11 May 2017 17:52:38 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> The script “checkpatch.pl” pointed information out like the following.
> 
> Comparison to NULL could be written …
> 
> Thus fix the affected source code places.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Please don't do these kind of checkpatch "fix ups" on existing code.
The comparison with NULL is fine, doing this is just useless churn.