| Message ID | 1270554249-24861-7-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 06, 2010 at 01:44:06PM +0200, Stefan Weil wrote: > To emulate hardware without an EEPROM, > EEPROM_SIZE may be set to 0. > > Signed-off-by: Stefan Weil <weil@mail.berlios.de> > --- > hw/eepro100.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/hw/eepro100.c b/hw/eepro100.c > index cedc427..e12ee23 100644 > --- a/hw/eepro100.c > +++ b/hw/eepro100.c > @@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev) > > e100_pci_reset(s, e100_device); > > +#if EEPROM_SIZE > 0 > /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, > * i82559 and later support 64 or 256 word EEPROM. */ > s->eeprom = eeprom93xx_new(EEPROM_SIZE); > +#endif I expect long-term EEPROM_SIZE will stop being a compile-time constant then? > > /* Handler for memory-mapped I/O */ > s->mmio_index = > -- > 1.7.0
Michael S. Tsirkin schrieb: > On Tue, Apr 06, 2010 at 01:44:06PM +0200, Stefan Weil wrote: > >> To emulate hardware without an EEPROM, >> EEPROM_SIZE may be set to 0. >> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de> >> --- >> hw/eepro100.c | 2 ++ >> 1 files changed, 2 insertions(+), 0 deletions(-) >> >> diff --git a/hw/eepro100.c b/hw/eepro100.c >> index cedc427..e12ee23 100644 >> --- a/hw/eepro100.c >> +++ b/hw/eepro100.c >> @@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev) >> >> e100_pci_reset(s, e100_device); >> >> +#if EEPROM_SIZE > 0 >> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, >> * i82559 and later support 64 or 256 word EEPROM. */ >> s->eeprom = eeprom93xx_new(EEPROM_SIZE); >> +#endif >> > > I expect long-term EEPROM_SIZE will stop being a compile-time > constant then? > > EEPROM_SIZE might be a qdev parameter, so a new eeprom_size would become part of the device status. Up to now, there was no need for it. >> >> /* Handler for memory-mapped I/O */ >> s->mmio_index = >> -- >> 1.7.0 >>
On 04/06/2010 04:44 AM, Stefan Weil wrote: > +#if EEPROM_SIZE > 0 > /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, > * i82559 and later support 64 or 256 word EEPROM. */ > s->eeprom = eeprom93xx_new(EEPROM_SIZE); > +#endif If EEPROM_SIZE is known to be defined, even if zero, it is better to write this as a C if, not a preprocessor ifdef. Let the compiler eliminate the dead code for you. r~
Richard Henderson schrieb: > On 04/06/2010 04:44 AM, Stefan Weil wrote: > >> +#if EEPROM_SIZE > 0 >> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, >> * i82559 and later support 64 or 256 word EEPROM. */ >> s->eeprom = eeprom93xx_new(EEPROM_SIZE); >> +#endif >> > > If EEPROM_SIZE is known to be defined, even if zero, it is > better to write this as a C if, not a preprocessor ifdef. > Let the compiler eliminate the dead code for you. > > > r~ > Why do you think that a C if is better than a CPP if in this case? Some compilers give warnings for code which will never be reached. Try gcc -Wunreachable-code. It's a really useful option which sometimes even detects programming errors, so maybe it would be good for qemu, too. Stefan
On 04/06/2010 09:01 AM, Stefan Weil wrote: > Richard Henderson schrieb: >> On 04/06/2010 04:44 AM, Stefan Weil wrote: >> >>> +#if EEPROM_SIZE > 0 >>> /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, >>> * i82559 and later support 64 or 256 word EEPROM. */ >>> s->eeprom = eeprom93xx_new(EEPROM_SIZE); >>> +#endif >>> ... > Why do you think that a C if is better than a CPP if > in this case? > > Some compilers give warnings for code which will > never be reached. Try gcc -Wunreachable-code. > It's a really useful option which sometimes even > detects programming errors, so maybe it would > be good for qemu, too. Having the compiler detect syntax and type mismatch errors in all code paths is generally far more valuable than warnings for unreachable code. These tend to creep in very easily with ifdef'ed code. This has follow-on effects as well. Suppose this instance above is the only place that eeprom93xx_new is called. With an ifdef here at the use site, the compiler will complain that eeprom93xx_new is unused, leading you to introduce more ifdefs, which means that even more code is unchecked. If you use a C if, then the compiler will see that eeprom93xx_new is used under other circumstances, not complain, and eliminate it as unused -- after having verified that it is both syntactically and semantically correct. Preprocessor spaghetti code is extremely hard to read. I know that QEMU is chock full of it, but let's try to reduce that rather than introduce more. r~
> To emulate hardware without an EEPROM, > EEPROM_SIZE may be set to 0. If might, but it isn't. This patch introduces a condition that will never be false. Please don't do that. I consider code that is never used to be actively harmful. Any feature that requires the user hack the source may as well not exist. The only possible exception is debug output intended solely for qemu developers. If there's something worth noting for future reference then add a proper comment, if necessary marked as TODO/FIXME. Paul
Paul Brook schrieb: >> To emulate hardware without an EEPROM, >> EEPROM_SIZE may be set to 0. > > If might, but it isn't. > > This patch introduces a condition that will never be false. Please > don't do > that. I consider code that is never used to be actively harmful. Any > feature > that requires the user hack the source may as well not exist. The only > possible exception is debug output intended solely for qemu developers. > > If there's something worth noting for future reference then add a proper > comment, if necessary marked as TODO/FIXME. > > Paul In this case, it is code which is normally always used, so maybe it is a little less harmful :-) Anyway - Richard already gave a good feedback on the same topic. His feedback convinced me that adding an eeprom size or model property as a device option would be the better way to support both developer needs (I want to make tests with no eeprom) and user needs (normally, an eeprom is available). The preprocessor #if would be replaced by a normal C if. I'll do this in a future patch. Michael, I suggest either omitting this patch or adding a TODO comment to the preprocessor #if like this: #if EEPROM_SIZE > 0 /* TODO: add a new EEPROM property and use it here */ Regards, Stefan
On Wed, Apr 07, 2010 at 09:02:06AM +0200, Stefan Weil wrote: > Paul Brook schrieb: > >> To emulate hardware without an EEPROM, > >> EEPROM_SIZE may be set to 0. > > > > If might, but it isn't. > > > > This patch introduces a condition that will never be false. Please > > don't do > > that. I consider code that is never used to be actively harmful. Any > > feature > > that requires the user hack the source may as well not exist. The only > > possible exception is debug output intended solely for qemu developers. > > > > If there's something worth noting for future reference then add a proper > > comment, if necessary marked as TODO/FIXME. > > > > Paul > > In this case, it is code which is normally always used, > so maybe it is a little less harmful :-) > > Anyway - Richard already gave a good feedback on the same topic. > > His feedback convinced me that adding an eeprom size or model > property as a device option would be the better way to > support both developer needs (I want to make tests with > no eeprom) and user needs (normally, an eeprom is > available). The preprocessor #if would be replaced by > a normal C if. > > I'll do this in a future patch. > > Michael, I suggest either omitting this patch or adding a > TODO comment to the preprocessor #if like this: > > #if EEPROM_SIZE > 0 /* TODO: add a new EEPROM property and use it here */ > > Regards, > Stefan I'll omit the patch. Thanks!
diff --git a/hw/eepro100.c b/hw/eepro100.c index cedc427..e12ee23 100644 --- a/hw/eepro100.c +++ b/hw/eepro100.c @@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev) e100_pci_reset(s, e100_device); +#if EEPROM_SIZE > 0 /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM, * i82559 and later support 64 or 256 word EEPROM. */ s->eeprom = eeprom93xx_new(EEPROM_SIZE); +#endif /* Handler for memory-mapped I/O */ s->mmio_index =
To emulate hardware without an EEPROM, EEPROM_SIZE may be set to 0. Signed-off-by: Stefan Weil <weil@mail.berlios.de> --- hw/eepro100.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)