Re: [Qemu-devel] [PATCH 01/13] qdev: rework device properties.

[Gerd Hoffmann <kraxel@redhat.com>]

On 07/10/09 19:13, Paul Brook wrote:
>> +extern PropertyInfo qdev_prop_uint32;
>> +extern PropertyInfo qdev_prop_hex32;
>
> Having both of these seems wrong.

Why?

There are properties which tend to be specified as decimal numbers 
(counts, sizes, ...) and some which tend to be specified in hex 
(adresses, ioports, ...).  I think it is useful to have separate 
parse/print functions for them, although they both end up being an uint32_t.

>> +            .name   = "fifo-size",
>> +            .info   =&qdev_prop_uint32,
>> +            .offset = offsetof(SyborgPointerState, fifo_size),
>> +            .defval = (uint32_t[]) { 16 },
>
> This feels kinda crufty. Very easy to get the wrong types.

I think I can make defval a string instead, then go through 
PropertyInfo->parse().  OK?

> There's also no
> typechecking in qdev_set_prop*.

It checks the size.  Which will miss some type mismatches.  I'll fix it.

>> +int qdev_prop_set_uint32(DeviceState *dev, const char *name, uint32_t
>> value);
>
> Why does this return a value?

It can fail if the size check (soon to be type check) mentioned above 
failed.  Such a failure would be a clear qemu bug though, so maybe 
abort() instead?

cheers,
   Gerd