[PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Jan 2017 16:52:23 +0100

A local variable was set to an error code in two cases before a concrete
error situation was detected. Thus move the corresponding assignments into
if branches to indicate a software failure there.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 arch/s390/kvm/kvm-s390.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4f74511015b8..bc875d08b838 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -443,16 +443,17 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	int is_dirty = 0;
 
 	mutex_lock(&kvm->slots_lock);
-
-	r = -EINVAL;
-	if (log->slot >= KVM_USER_MEM_SLOTS)
+	if (log->slot >= KVM_USER_MEM_SLOTS) {
+		r = -EINVAL;
 		goto out;
+	}
 
 	slots = kvm_memslots(kvm);
 	memslot = id_to_memslot(slots, log->slot);
-	r = -ENOENT;
-	if (!memslot->dirty_bitmap)
+	if (!memslot->dirty_bitmap) {
+		r = -ENOENT;
 		goto out;
+	}
 
 	kvm_s390_sync_dirty_log(kvm, memslot);
 	r = kvm_get_dirty_log(kvm, log, &is_dirty);
-- 
2.11.0

Re: [PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Christian Borntraeger]

On 01/21/2017 05:03 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 21 Jan 2017 16:52:23 +0100
> 
> A local variable was set to an error code in two cases before a concrete
> error situation was detected. Thus move the corresponding assignments into
> if branches to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>


Patches that changes open coded things to common helpers or things like
kmalloc_array where appropriate or things that make the code more robust
are fine and welcome, but I am not going to take this as it just shuffles
things around. It does not fix anything and it does not improve the code,
but it certainly carries the risk of breaking something (yes in this case
it looks perfectly fine, though).

PS: When you look at the kvm/mips change that you have recently made, this 
case is better and qualifies as in improvement, because you remove lines and 
the code does get simpler. Due to the locking requirements we cannot do
such a simplification here.

Christian

> ---
>  arch/s390/kvm/kvm-s390.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 4f74511015b8..bc875d08b838 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -443,16 +443,17 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  	int is_dirty = 0;
> 
>  	mutex_lock(&kvm->slots_lock);
> -
> -	r = -EINVAL;
> -	if (log->slot >= KVM_USER_MEM_SLOTS)
> +	if (log->slot >= KVM_USER_MEM_SLOTS) {
> +		r = -EINVAL;
>  		goto out;
> +	}
> 
>  	slots = kvm_memslots(kvm);
>  	memslot = id_to_memslot(slots, log->slot);
> -	r = -ENOENT;
> -	if (!memslot->dirty_bitmap)
> +	if (!memslot->dirty_bitmap) {
> +		r = -ENOENT;
>  		goto out;
> +	}
> 
>  	kvm_s390_sync_dirty_log(kvm, memslot);
>  	r = kvm_get_dirty_log(kvm, log, &is_dirty);
> 

Re: [PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Paolo Bonzini]

On 23/01/2017 09:22, Christian Borntraeger wrote:
> Patches that changes open coded things to common helpers or things like
> kmalloc_array where appropriate or things that make the code more robust
> are fine and welcome, but I am not going to take this as it just shuffles
> things around. It does not fix anything and it does not improve the code,
> but it certainly carries the risk of breaking something (yes in this case
> it looks perfectly fine, though).

Besides,

	r = -ESOMETHING;
	if (...)
		return r;

is the way it's done everywhere else in KVM.

Paolo

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

> Patches that changes open coded things to common helpers or things like
> kmalloc_array where appropriate or things that make the code more robust
> are fine and welcome, but I am not going to take this as it just shuffles
> things around.

Thanks for such information.


> It does not fix anything and it does not improve the code,

I have got an other expectation for the shown implementation detail.


> but it certainly carries the risk of breaking something

This is usual in software development, isn't it?


> (yes in this case it looks perfectly fine, though).

Thanks for this bit of positive feedback.


> Due to the locking requirements we cannot do such a simplification here.

I find this detail strange. Would you like to check run time consequences
for the shown error code settings once more?

Regards,
Markus

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Christian Borntraeger]

On 01/23/2017 12:08 PM, SF Markus Elfring wrote:
> Would you like to check run time consequences
> for the shown error code settings once more?

Sure, lets for now ignore the fact that the performance of an error path
does not matter most of the time.

Have you actually looked at your patch? After tree building and optimization
your change should not matter at all regarding performance for a decent
compiler. The compiler can and will do much more complex transformations
than this.

Since you have send several patches that trigger compile time warnings or
errors, let me do this exercise for you and let us check what your patch
changes in terms of run time consequences.


$ git checkout v4.10-rc4
HEAD is now at 49def18... Linux 4.10-rc4

$ make arch/s390/kvm/kvm-s390.o
[..]

$ objdump -d arch/s390/kvm/kvm-s390.o | md5sum
55c1e081f55cef90b3ffcc06a13721c1  -

$ git am ~/code/elfring/[PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log().eml
Applying: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

$ make arch/s390/kvm/kvm-s390.o
[..]

$ objdump -d arch/s390/kvm/kvm-s390.o | md5sum
55c1e081f55cef90b3ffcc06a13721c1  -

As you can see the binary is identical, so I can make an educated guess, that 
there is no performance improvement due to your patch. The bad news is that 
there are reasons to not apply this patch as outlined by me and by Paolo.

Can we come back to a point, where you accept feedback?

Christian

PS: and maybe you should also start to test your patches in a cross-compile
environment before submitting or - heaven forbid - maybe even test your
changes.

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

>> Would you like to check run time consequences
>> for the shown error code settings once more?
> 
> Sure, lets for now ignore the fact that the performance of an error path
> does not matter most of the time.

I am concerned that extra error code settings within the “success path”
could influence the run time behaviour in unwanted ways.


> After tree building and optimization your change should not matter at all
> regarding performance for a decent compiler.

I find your optimism interesting.


> The compiler can and will do much more complex transformations than this.

This technology is often fine.


> Since you have send several patches that trigger compile time warnings or
> errors, let me do this exercise for you and let us check what your patch
> changes in terms of run time consequences.
> 
> 
> $ git checkout v4.10-rc4
> HEAD is now at 49def18... Linux 4.10-rc4
> 
> $ make arch/s390/kvm/kvm-s390.o
> [..]


Thanks for your build demonstration.


> $ objdump -d arch/s390/kvm/kvm-s390.o | md5sum
> 55c1e081f55cef90b3ffcc06a13721c1  -
> 
> $ git am ~/code/elfring/[PATCH] KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log().eml
> Applying: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()
> 
> $ make arch/s390/kvm/kvm-s390.o
> [..]
> 
> $ objdump -d arch/s390/kvm/kvm-s390.o | md5sum
> 55c1e081f55cef90b3ffcc06a13721c1  -
> 
> As you can see the binary is identical,

The hashes became the same with the selected tool.


> so I can make an educated guess, that there is no performance improvement
> due to your patch.

How much does such a software generation result fit really to expectations?

Should the two shown implementation variants for a function like "kvm_vm_ioctl_get_dirty_log"
usually lead to different object code files?

Regards,
Markus

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Paolo Bonzini]

On 24/01/2017 13:10, SF Markus Elfring wrote:
>>> Would you like to check run time consequences
>>> for the shown error code settings once more?
>>
>> Sure, lets for now ignore the fact that the performance of an error path
>> does not matter most of the time.
> 
> I am concerned that extra error code settings within the “success path”
> could influence the run time behaviour in unwanted ways.
> 
> 
>> After tree building and optimization your change should not matter at all
>> regarding performance for a decent compiler.
> 
> I find your optimism interesting.

Mummy says stupid is as stupid does.  Sorry, but I'm putting you on a
kill list.  It's just not possible to discuss things with you. You're
the first person _ever_ to get this treatment in about 20 years of me
using the Internet.

If other people want to merge your patches to KVM, they are free to do so.

Paolo

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

> It's just not possible to discuss things with you.

I wonder about your conclusion. We have got just different software
development opinions on some details.

I assume that a few additional dialogue techniques could help a bit more
in our communication difficulties.

 
> If other people want to merge your patches to KVM, they are free to do so.

I am curious if other update suggestions will be picked up
in a constructive way.

Regards,
Markus

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Christian Borntraeger]

On 01/24/2017 01:31 PM, SF Markus Elfring wrote:
>> It's just not possible to discuss things with you.
> 
> I wonder about your conclusion. We have got just different software
> development opinions on some details.
> 
> I assume that a few additional dialogue techniques could help a bit more
> in our communication difficulties.

I will also start to ignore all mails from you.
> 
> 
>> If other people want to merge your patches to KVM, they are free to do so.
> 
> I am curious if other update suggestions will be picked up
> in a constructive way.

There is only one person on this list that is not constructive, and that
person is you.

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

>> I am curious if other update suggestions will be picked up
>> in a constructive way.
> 
> There is only one person on this list that is not constructive,

You could accept a few patches from my selection of change possibilities already.


> and that person is you.

I assume that some dialogue steps would be needed to find a “common wave length” again
so that the communication might become useful also in your view.

Regards,
Markus

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Dan Carpenter]

On Mon, Jan 23, 2017 at 12:08:12PM +0100, SF Markus Elfring wrote:
> > but it certainly carries the risk of breaking something
> 
> This is usual in software development, isn't it?
> 

If you break something but you're trying to fix something then normally
you still end up fixing more than you break.  You haven't yet tried to
fix anything, but you've definitely introduced bugs...

We have had this discussion many times.

regards,
dan carpenter

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[SF Markus Elfring]

> You haven't yet tried to fix anything,

I have got an other impression.


> but you've definitely introduced bugs...

I am also stumbling on my own share of software development mistakes.


> We have had this discussion many times.

The usual challenges remain from different opinions about involved details.

Regards,
Markus

Re: KVM: s390: Move two error code assignments in kvm_vm_ioctl_get_dirty_log()

[Dan Carpenter]

On Mon, Jan 23, 2017 at 02:21:45PM +0100, SF Markus Elfring wrote:
> > You haven't yet tried to fix anything,
> 
> I have got an other impression.
> 

Name three bugs that you have fixed (not cleanups).

regards,
dan carpenter