Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Philippe Mathieu-Daudé]

Hi Wolfram,

On 03/13/2018 09:16 PM, Wolfram Sang wrote:
> Hi Philippe,
> 
>>>  static Property at24c_eeprom_props[] = {
>>> -    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
>>> +    DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
>>
>> This patch should goes before your 2/3 in your series.
> 
> I don't mind much, but why? My reasoning was "let's first fix the cause
> and then the symptom"?

The '0' case is worst than incorrect, it segfaults, so you are right :)

> 
>> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> 
> Can do, of course. But won't that give room for regressions because
> people are already using it with lower values?

Your patch already introduce the regression :)

I prefer self-explanatory #defines than magic value, but I see your
point, so if we can not decide a value, can you add a comment to explain
the magic value? I think the clearer is to add a #define with a comment.

> 
> Ideally, we would have a "model" variable. The model type would define
> the size of the memory. The "rom-size" variable could then be kept as is
> (except for the 0 bugfix) or deprecated?

IMO there are too many AT24C eeproms to model, so the "rom-size"
variable is the easiest way. Your patch #2 is simple enough to avoid the
#DIV/0!

> 
> Thanks for the review,
> 
>    Wolfram
>

signature.asc
Description: OpenPGP digital signature