[Qemu-devel] [PATCH 0/3] Fix broken if statements

[Qemu-devel] [PATCH 0/3] Fix broken if statements

[Joel Schopp]

I have decided to apply the broken window theory of crime 
http://en.wikipedia.org/wiki/Broken_windows_theory to code, and
more specifically to qemu.  I'm hoping that fixing seemingly trivial
bugs will actually fix some more serious bugs, make the code run
just a bit smoother, or at the very least leave us with easier to
maintain code.

Joel Schopp (3):
  remove dead code from hw/loader.c
  fix variable type in qemu-io.c
  remove pointless if from vl.c

 hw/loader.c |    5 -----
 qemu-io.c   |    4 ++--
 vl.c        |    4 +---
 3 files changed, 3 insertions(+), 10 deletions(-)

[Qemu-devel] [PATCH 1/3] remove dead code from hw/loader.c

[Joel Schopp]

Removing dead code.  Above we already continued when
rom->addr + valuegreaterthan0 < addr so this condition is always false.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com>
---
 hw/loader.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 79a6f95..49ac1fa 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -733,11 +733,6 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size)
         s = rom->data;
         l = rom->romsize;
 
-        if (rom->addr < addr) {
-            d = dest;
-            s += (addr - rom->addr);
-            l -= (addr - rom->addr);
-        }
         if ((d + l) > (dest + size)) {
             l = dest - d;
         }
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/3] fix variable type in qemu-io.c

[Joel Schopp]

The variable len can get a negative return value from cvtnum,
which we check for, but which is impossible with the current
unsigned variable type.  Currently the if(len < 0) check is
pointless.  This patch fixes that.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com>
---
 qemu-io.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index 9adda4c..1a9c13d 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -135,7 +135,7 @@ create_iovec(QEMUIOVector *qiov, char **argv, int nr_iov, int pattern)
 
 	for (i = 0; i < nr_iov; i++) {
 		char *arg = argv[i];
-                uint64_t len;
+                int64_t len;
 
 		len = cvtnum(arg);
 		if (len < 0) {
@@ -144,7 +144,7 @@ create_iovec(QEMUIOVector *qiov, char **argv, int nr_iov, int pattern)
 		}
 
 		/* should be SIZE_T_MAX, but that doesn't exist */
-		if (len > UINT_MAX) {
+		if (len > INT_MAX) {
 			printf("too large length argument -- %s\n", arg);
 			goto fail;
 		}
-- 
1.7.0.4

[Qemu-devel] [PATCH 3/3] remove pointless if from vl.c

[Joel Schopp]

We already set sockets to nonzero in the code above.
So this if statement always evaluates true.  Remove it.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com>
---
 vl.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index 8a5de9f..a0c28b6 100644
--- a/vl.c
+++ b/vl.c
@@ -801,9 +801,7 @@ static void smp_parse(const char *optarg)
             threads = threads > 0 ? threads : 1;
             cores = smp / (sockets * threads);
         } else {
-            if (sockets) {
-                threads = smp / (cores * sockets);
-            }
+            threads = smp / (cores * sockets);
         }
     }
     smp_cpus = smp;
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 0/3] Fix broken if statements

[Stefan Weil]

Am 21.07.2010 22:05, schrieb Joel Schopp:
> I have decided to apply the broken window theory of crime
> http://en.wikipedia.org/wiki/Broken_windows_theory to code, and
> more specifically to qemu. I'm hoping that fixing seemingly trivial
> bugs will actually fix some more serious bugs, make the code run
> just a bit smoother, or at the very least leave us with easier to
> maintain code.
>
> Joel Schopp (3):
> remove dead code from hw/loader.c
> fix variable type in qemu-io.c
> remove pointless if from vl.c
>
> hw/loader.c | 5 -----
> qemu-io.c | 4 ++--
> vl.c | 4 +---
> 3 files changed, 3 insertions(+), 10 deletions(-)


How did you apply the broken window theory of crime to software code?
Is this still a secret, or can you tell us more?

Regards
Stefan Weil

Re: [Qemu-devel] [PATCH 0/3] Fix broken if statements

[Aurelien Jarno]

On Wed, Jul 21, 2010 at 03:05:14PM -0500, Joel Schopp wrote:
> I have decided to apply the broken window theory of crime 
> http://en.wikipedia.org/wiki/Broken_windows_theory to code, and
> more specifically to qemu.  I'm hoping that fixing seemingly trivial
> bugs will actually fix some more serious bugs, make the code run
> just a bit smoother, or at the very least leave us with easier to
> maintain code.
> 
> Joel Schopp (3):
>   remove dead code from hw/loader.c
>   fix variable type in qemu-io.c
>   remove pointless if from vl.c
> 
>  hw/loader.c |    5 -----
>  qemu-io.c   |    4 ++--
>  vl.c        |    4 +---
>  3 files changed, 3 insertions(+), 10 deletions(-)
> 
> 
> 

Thanks, all 3 patches applied.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/3] Fix broken if statements

[Andreas Färber]

Am 22.07.2010 um 18:18 schrieb Stefan Weil:

> Am 21.07.2010 22:05, schrieb Joel Schopp:
>> I have decided to apply the broken window theory of crime
>> http://en.wikipedia.org/wiki/Broken_windows_theory to code, and
>> more specifically to qemu[:]

>> I'm hoping that fixing seemingly trivial
>> bugs will actually fix some more serious bugs, make the code run
>> just a bit smoother, or at the very least leave us with easier to
>> maintain code.

>> Joel Schopp (3):
>> remove dead code from hw/loader.c
>> fix variable type in qemu-io.c
>> remove pointless if from vl.c
[...]
> How did you apply the broken window theory of crime to software code?
> Is this still a secret, or can you tell us more?

If you consider Coding Style and Best Practice violations a crime  
(vandalism / broken window), then e.g. removing dead code is to  
prevent future code dead-on-arrival and - as criminal escalation - bugs.

Applying it further might mean that someone could provide a patch  
either adding or removing braces for all single line statements in the  
code base so that the broken window argument (someone broke the rule  
so I can, too) no longer holds for new patches arriving. :)

Regards,
Andreas

Re: [Qemu-devel] [PATCH 0/3] Fix broken if statements

[Stefan Weil]

Am 01.08.2010 19:25, schrieb Andreas Färber:
> Am 22.07.2010 um 18:18 schrieb Stefan Weil:
>
>> Am 21.07.2010 22:05, schrieb Joel Schopp:
>>> I have decided to apply the broken window theory of crime
>>> http://en.wikipedia.org/wiki/Broken_windows_theory to code, and
>>> more specifically to qemu[:]
>
>>> I'm hoping that fixing seemingly trivial
>>> bugs will actually fix some more serious bugs, make the code run
>>> just a bit smoother, or at the very least leave us with easier to
>>> maintain code.
>
>>> Joel Schopp (3):
>>> remove dead code from hw/loader.c
>>> fix variable type in qemu-io.c
>>> remove pointless if from vl.c
> [...]
>> How did you apply the broken window theory of crime to software code?
>> Is this still a secret, or can you tell us more?
>
> If you consider Coding Style and Best Practice violations a crime 
> (vandalism / broken window), then e.g. removing dead code is to 
> prevent future code dead-on-arrival and - as criminal escalation - bugs.
>
> Applying it further might mean that someone could provide a patch 
> either adding or removing braces for all single line statements in the 
> code base so that the broken window argument (someone broke the rule 
> so I can, too) no longer holds for new patches arriving. :)
>
> Regards,
> Andreas
>

Agreed.

I should have been more precise in formulating my question:

Is there some magic (= tool) which detected these "broken windows"
in hw/loader.c, qemu-io.c and vl.c, or was it just a manual code
review or luck?

It's not too difficult to write a tool which detects most typical violations
of coding style (Aurelien suggested such a filter for git), but Joel's 
"broken
windows" are less obvious.

Regards
Stefan

Re: [Qemu-devel] [PATCH 0/3] Fix broken if statements

[Joel Schopp]

 > Is there some magic (= tool) which detected these "broken windows"
 > in hw/loader.c, qemu-io.c and vl.c, or was it just a manual code
 > review or luck?

I used a proprietary static analysis tool called BEAM.  
http://domino.research.ibm.com/comm/research.nsf/pages/r.da.beam.html
It found pages of potential errors, about 80% of which seem valid.  
Fixing the bugs with obvious fixes seems like a good way for me to learn 
the qemu code while providing a useful service at the same time.  If 
anybody wants to see the output of the tool (plenty of bugs to go 
around) please email me off list.  Some of the bugs it found, I'm 
thinking of out of bound array accesses and returning pointers to stack 
variables, probably have security implications so I'd like to not share 
those publicly until there are patches to fix them.