[Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

[Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

[Thomas Huth]

Remove the unnecessary home-grown redefinition of the assert() macro here,
and remove the unusable debug code at the end of the checkpoint() function.
The code there uses assert() with side-effects (assignment to the "mapping"
variable), which should be avoided. Looking more closely, it seems as it is
apparently also only usable for one certain directory layout (with a file
named USB.H in it) and thus is of no use for the rest of the world.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 block/vvfat.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index c54fa94..777a8cd 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -56,15 +56,6 @@
 
 static void checkpoint(void);
 
-#ifdef __MINGW32__
-void nonono(const char* file, int line, const char* msg) {
-    fprintf(stderr, "Nonono! %s:%d %s\n", file, line, msg);
-    exit(-5);
-}
-#undef assert
-#define assert(a) do {if (!(a)) nonono(__FILE__, __LINE__, #a);}while(0)
-#endif
-
 #else
 
 #define DLOG(a)
@@ -3269,24 +3260,11 @@ static void bdrv_vvfat_init(void)
 block_init(bdrv_vvfat_init);
 
 #ifdef DEBUG
-static void checkpoint(void) {
+static void checkpoint(void)
+{
     assert(((mapping_t*)array_get(&(vvv->mapping), 0))->end == 2);
     check1(vvv);
     check2(vvv);
     assert(!vvv->current_mapping || vvv->current_fd || (vvv->current_mapping->mode & MODE_DIRECTORY));
-#if 0
-    if (((direntry_t*)vvv->directory.pointer)[1].attributes != 0xf)
-        fprintf(stderr, "Nonono!\n");
-    mapping_t* mapping;
-    direntry_t* direntry;
-    assert(vvv->mapping.size >= vvv->mapping.item_size * vvv->mapping.next);
-    assert(vvv->directory.size >= vvv->directory.item_size * vvv->directory.next);
-    if (vvv->mapping.next<47)
-        return;
-    assert((mapping = array_get(&(vvv->mapping), 47)));
-    assert(mapping->dir_index < vvv->directory.next);
-    direntry = array_get(&(vvv->directory), mapping->dir_index);
-    assert(!memcmp(direntry->name, "USB     H  ", 11) || direntry->name[0]==0);
-#endif
 }
 #endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On 09/13/2017 05:21 AM, Thomas Huth wrote:
> Remove the unnecessary home-grown redefinition of the assert() macro here,
> and remove the unusable debug code at the end of the checkpoint() function.
> The code there uses assert() with side-effects (assignment to the "mapping"
> variable), which should be avoided. Looking more closely, it seems as it is
> apparently also only usable for one certain directory layout (with a file
> named USB.H in it) and thus is of no use for the rest of the world.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  block/vvfat.c | 26 ++------------------------
>  1 file changed, 2 insertions(+), 24 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver

[John Snow]

On 09/13/2017 06:21 AM, Thomas Huth wrote:
> Remove the unnecessary home-grown redefinition of the assert() macro here,
> and remove the unusable debug code at the end of the checkpoint() function.
> The code there uses assert() with side-effects (assignment to the "mapping"
> variable), which should be avoided. Looking more closely, it seems as it is
> apparently also only usable for one certain directory layout (with a file
> named USB.H in it) and thus is of no use for the rest of the world.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Farewell, bitrot code.

Reviewed-by: John Snow <jsnow@redhat.com>

Out of curiosity, I wonder ...

jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
320

oh no

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver

[Paolo Bonzini]

On 13/09/2017 21:08, John Snow wrote:
> 
> 
> On 09/13/2017 06:21 AM, Thomas Huth wrote:
>> Remove the unnecessary home-grown redefinition of the assert() macro here,
>> and remove the unusable debug code at the end of the checkpoint() function.
>> The code there uses assert() with side-effects (assignment to the "mapping"
>> variable), which should be avoided. Looking more closely, it seems as it is
>> apparently also only usable for one certain directory layout (with a file
>> named USB.H in it) and thus is of no use for the rest of the world.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Farewell, bitrot code.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Out of curiosity, I wonder ...
> 
> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
> 320


$ git grep -c '#if 0' | sort -k2 --field-separator=: -n
...
hw/net/eepro100.c:21
target/ppc/cpu-models.h:76

whoa :)

[Qemu-devel] Dead code in cpu-models.h (was: block: Clean up some bad code in the vvfat driver)

[Thomas Huth]

On 19.09.2017 10:06, Paolo Bonzini wrote:
> On 13/09/2017 21:08, John Snow wrote:
[...]
>> Farewell, bitrot code.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> Out of curiosity, I wonder ...
>>
>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>> 320
> 
> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
> ...
> hw/net/eepro100.c:21
> target/ppc/cpu-models.h:76
> 
> whoa :)

Igor recently already removed the dead definitions from cpu-models.c :

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff

I guess we could now remove the corresponding dead definitions from the
header, too...

Any volunteers?

 Thomas

Re: [Qemu-devel] Dead code in cpu-models.h

[John Snow]

On 09/19/2017 04:14 AM, Thomas Huth wrote:
> On 19.09.2017 10:06, Paolo Bonzini wrote:
>> On 13/09/2017 21:08, John Snow wrote:
> [...]
>>> Farewell, bitrot code.
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>> Out of curiosity, I wonder ...
>>>
>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>>> 320
>>
>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
>> ...
>> hw/net/eepro100.c:21
>> target/ppc/cpu-models.h:76
>>
>> whoa :)
> 
> Igor recently already removed the dead definitions from cpu-models.c :
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff
> 
> I guess we could now remove the corresponding dead definitions from the
> header, too...
> 
> Any volunteers?
> 
>  Thomas
> 

Well, I can just take a chainsaw to it blindly if you want to critique it.

Re: [Qemu-devel] Dead code in cpu-models.h

[Thomas Huth]

On 19.09.2017 20:54, John Snow wrote:
> 
> 
> On 09/19/2017 04:14 AM, Thomas Huth wrote:
>> On 19.09.2017 10:06, Paolo Bonzini wrote:
>>> On 13/09/2017 21:08, John Snow wrote:
>> [...]
>>>> Farewell, bitrot code.
>>>>
>>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>>
>>>> Out of curiosity, I wonder ...
>>>>
>>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>>>> 320
>>>
>>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
>>> ...
>>> hw/net/eepro100.c:21
>>> target/ppc/cpu-models.h:76
>>>
>>> whoa :)
>>
>> Igor recently already removed the dead definitions from cpu-models.c :
>>
>> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=aef779605779579afbafff
>>
>> I guess we could now remove the corresponding dead definitions from the
>> header, too...
>>
>> Any volunteers?
>>
>>  Thomas
> 
> Well, I can just take a chainsaw to it blindly if you want to critique it.
> 

I think it should at least be aligned with the changes that have been
done to cpu-models.c (e.g. the definition for CPU_POWERPC_401B3 has been
removed from the .c file, so the CPU_POWERPC_401B3 could be removed from
the .h file, too.

 Thomas

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver

[John Snow]

On 09/19/2017 04:06 AM, Paolo Bonzini wrote:
> On 13/09/2017 21:08, John Snow wrote:
>>
>>
>> On 09/13/2017 06:21 AM, Thomas Huth wrote:
>>> Remove the unnecessary home-grown redefinition of the assert() macro here,
>>> and remove the unusable debug code at the end of the checkpoint() function.
>>> The code there uses assert() with side-effects (assignment to the "mapping"
>>> variable), which should be avoided. Looking more closely, it seems as it is
>>> apparently also only usable for one certain directory layout (with a file
>>> named USB.H in it) and thus is of no use for the rest of the world.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>
>> Farewell, bitrot code.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
>> Out of curiosity, I wonder ...
>>
>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>> 320
> 
> 
> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
> ...
> hw/net/eepro100.c:21
> target/ppc/cpu-models.h:76
> 
> whoa :)
> 

Wonder if '#if 0' should be against the style guide / in checkpatch.
Conditional compilations should at least be contingent on some named
variable/condition.

(Probably super ideally all conditional compilations correlate directly
to a configure variable...)

--js

Re: [Qemu-devel] [Qemu-block] [PATCH] block: Clean up some bad code in the vvfat driver

[Thomas Huth]

On 19.09.2017 21:01, John Snow wrote:
> 
> 
> On 09/19/2017 04:06 AM, Paolo Bonzini wrote:
>> On 13/09/2017 21:08, John Snow wrote:
>>>
>>>
>>> On 09/13/2017 06:21 AM, Thomas Huth wrote:
>>>> Remove the unnecessary home-grown redefinition of the assert() macro here,
>>>> and remove the unusable debug code at the end of the checkpoint() function.
>>>> The code there uses assert() with side-effects (assignment to the "mapping"
>>>> variable), which should be avoided. Looking more closely, it seems as it is
>>>> apparently also only usable for one certain directory layout (with a file
>>>> named USB.H in it) and thus is of no use for the rest of the world.
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> Farewell, bitrot code.
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>> Out of curiosity, I wonder ...
>>>
>>> jhuston@probe (foobar) ~/s/qemu> git grep '#if 0' | wc -l
>>> 320
>>
>>
>> $ git grep -c '#if 0' | sort -k2 --field-separator=: -n
>> ...
>> hw/net/eepro100.c:21
>> target/ppc/cpu-models.h:76
>>
>> whoa :)
>>
> 
> Wonder if '#if 0' should be against the style guide / in checkpatch.

checkpatch already complains if you have a "#if 0" in your patch, so I
think we should be pretty fine here already - but of course you can
still add a paragraph to the CODING_STYLE if you like.

 Thomas

Re: [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

[Kevin Wolf]

Am 13.09.2017 um 12:21 hat Thomas Huth geschrieben:
> Remove the unnecessary home-grown redefinition of the assert() macro here,
> and remove the unusable debug code at the end of the checkpoint() function.
> The code there uses assert() with side-effects (assignment to the "mapping"
> variable), which should be avoided. Looking more closely, it seems as it is
> apparently also only usable for one certain directory layout (with a file
> named USB.H in it) and thus is of no use for the rest of the world.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Thanks, applied to the block branch.

Kevin