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

[Paul Brook <paul@codesourcery.com>]

On Friday 10 July 2009, Gerd Hoffmann wrote:
> 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.

I think this is the wrong distinction. Whether you specify something in hex or 
decimal (or even binary/octal) is personal user preference.  Distinguishing 
between integers and addresses may make sense, as those actually have 
different types.

> >> +            .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?

I don't think that's really any better. I guess it may just be something we 
have to put up with. Possibly have a helper macro that sets all the fields. 
Something like:

  .properties = (Property[]) {
    DEFINE_PROPERTY_UINT32("fifo-size", SyborgPointerState, fifo_size, 16)
  }

Ideally we'd have a single DEFINE_PROPERTY macro and the type would be figured 
out from typeof(fifo_size), but I can't think how to do that.

> >> +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?

Yes. Returning a status code then never checking it is completely pointless.

Paul