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

2017-09-19 Thread 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 
>>>
>>> Farewell, bitrot code.
>>>
>>> Reviewed-by: John Snow 
>>>
>>> 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-block] [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

2017-09-19 Thread 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 
>>
>> Farewell, bitrot code.
>>
>> Reviewed-by: John Snow 
>>
>> 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-block] [Qemu-devel] [PATCH] block: Clean up some bad code in the vvfat driver

2017-09-13 Thread Eric Blake 
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 
> ---
>  block/vvfat.c | 26 ++
>  1 file changed, 2 insertions(+), 24 deletions(-)
> 

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature