[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 01/13] qdev: rework device properties.
From: |
Paul Brook |
Subject: |
Re: [Qemu-devel] [PATCH 01/13] qdev: rework device properties. |
Date: |
Fri, 10 Jul 2009 20:42:59 +0100 |
User-agent: |
KMail/1.11.4 (Linux/2.6.29-2-amd64; KDE/4.2.4; x86_64; ; ) |
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
- [Qemu-devel] [PATCH 02/13] qdev: factor out driver search to qdev_find_info(), (continued)
- [Qemu-devel] [PATCH 02/13] qdev: factor out driver search to qdev_find_info(), Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 03/13] qdev/pci: make pci_create return DeviceState instead of PCIDevice., Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 06/13] qdev: add no_user, alias and desc, Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 07/13] qdev: es1370 description, Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 08/13] qdev: convert all vga, Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 13/13] qdev: print device id in "info pci"., Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 09/13] qdev/pci: hook up i440fx., Gerd Hoffmann, 2009/07/10
- [Qemu-devel] [PATCH 01/13] qdev: rework device properties., Gerd Hoffmann, 2009/07/10
[Qemu-devel] [PATCH 10/13] qdev: add user-specified identifier to devices., Gerd Hoffmann, 2009/07/10
[Qemu-devel] [PATCH 12/13] qdev: add id= support for pci nics., Gerd Hoffmann, 2009/07/10
[Qemu-devel] [PATCH 05/13] qdev: add -device command line option., Gerd Hoffmann, 2009/07/10
[Qemu-devel] [PATCH 11/13] switch balloon initialization to -device., Gerd Hoffmann, 2009/07/10