Re: [PATCH v4 31/37] smbus-eeprom: remove PROP_PTR

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/22/19 12:12 PM, Marc-André Lureau wrote:
> On Fri, Nov 22, 2019 at 3:07 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> On 11/20/19 4:24 PM, Marc-André Lureau wrote:
>>> Instead, set the initial data field directly.
>>>
>>> (the initial data is an array of 256 bytes. As I don't know if it may
>>> change over time, I keep the pointer to original buffer as is, but it
>>> might be worth to consider to copy it instead)
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    hw/i2c/smbus_eeprom.c | 17 ++++++++---------
>>>    1 file changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
>>> index 54c86a0112..533c728b3b 100644
>>> --- a/hw/i2c/smbus_eeprom.c
>>> +++ b/hw/i2c/smbus_eeprom.c
>>> @@ -44,7 +44,7 @@
>>>    typedef struct SMBusEEPROMDevice {
>>>        SMBusDevice smbusdev;
>>>        uint8_t data[SMBUS_EEPROM_SIZE];
>>> -    void *init_data;
>>> +    uint8_t *init_data;
>>>        uint8_t offset;
>>>        bool accessed;
>>>    } SMBusEEPROMDevice;
>>> @@ -129,14 +129,14 @@ static void smbus_eeprom_reset(DeviceState *dev)
>>>
>>>    static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>>>    {
>>> +    SMBusEEPROMDevice *eeprom = SMBUS_EEPROM(dev);
>>> +
>>>        smbus_eeprom_reset(dev);
>>> +    if (eeprom->init_data == NULL) {
>>> +        error_setg(errp, "init_data cannot be NULL");
>>> +    }
>>>    }
>>>
>>> -static Property smbus_eeprom_properties[] = {
>>> -    DEFINE_PROP_PTR("data", SMBusEEPROMDevice, init_data),
>>> -    DEFINE_PROP_END_OF_LIST(),
>>> -};
>>> -
>>>    static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>>    {
>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>> @@ -146,9 +146,8 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>>>        dc->reset = smbus_eeprom_reset;
>>>        sc->receive_byte = eeprom_receive_byte;
>>>        sc->write_data = eeprom_write_data;
>>> -    dc->props = smbus_eeprom_properties;
>>>        dc->vmsd = &vmstate_smbus_eeprom;
>>> -    /* Reason: pointer property "data" */
>>> +    /* Reason: init_data */
>>>        dc->user_creatable = false;
>>>    }
>>>
>>> @@ -172,7 +171,7 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf)
>>>
>>>        dev = qdev_create((BusState *) smbus, TYPE_SMBUS_EEPROM);
>>>        qdev_prop_set_uint8(dev, "address", address);
>>> -    qdev_prop_set_ptr(dev, "data", eeprom_buf);
>>> +    SMBUS_EEPROM(dev)->init_data = eeprom_buf;
>>
>> Isn't this a QOM anti-pattern?
> 
> Sort of, but using prop_ptr in the first place is an anti-pattern. So
> I am not improving the situation much here, but just getting rid of
> prop_ptr usage.

Alright.

> Using an array of bytes instead could be done in a follow-up patch. I
> can leave a fixme if that helps.

IIRC there was a discussion earlier this year about using block backend 
for this device.

>>
>>>        qdev_init_nofail(dev);
>>>    }
>>>
>>>
>>
>