Patchwork [6/9] eepro100: Support compilation without EEPROM

login
register
mail settings
Submitter Stefan Weil
Date April 6, 2010, 11:44 a.m.
Message ID <1270554249-24861-7-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/49503/
State New
Headers show

Comments

Stefan Weil - April 6, 2010, 11:44 a.m.
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(-)
Michael S. Tsirkin - April 6, 2010, 12:10 p.m.
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
Stefan Weil - April 6, 2010, 2:26 p.m.
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
>>
Richard Henderson - April 6, 2010, 3:44 p.m.
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~
Stefan Weil - April 6, 2010, 4:01 p.m.
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
Richard Henderson - April 6, 2010, 4:35 p.m.
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~
Paul Brook - April 7, 2010, 1 a.m.
> 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
Stefan Weil - April 7, 2010, 7:02 a.m.
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
Michael S. Tsirkin - April 7, 2010, 7:29 a.m.
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!

Patch

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 =