Message ID | 1505298088-10878-1-git-send-email-thuth@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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>
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
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
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 :)
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
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.
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
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
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
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
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(-)