[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-s
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size" |
Date: |
Mon, 19 Mar 2018 00:50:30 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
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
[Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR, Wolfram Sang, 2018/03/12
[Qemu-devel] [PATCH 2/3] nvram: at24c: prevent segfault by checking "rom-size", Wolfram Sang, 2018/03/12