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
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
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
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 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 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 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
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
© 2016 - 2024 Red Hat, Inc.