On 02/14/2011 11:31 AM, Blue Swirl wrote:
I don't understand. The caller just does
if (isa_serial_init()) {
error();
}
or
if (serial_init()) {
error();
}
If you mean inside isa_serial_init() vs. serial_init(), that may be
true since isa_serial_init has to check for qdev failures, but the to
the caller both should be identical.
The problem with qdev is there's too much boiler plate code which makes
it hard to give examples :-) Here's precisely what I'm talking about:
static int serial_isa_initfn(ISADevice *dev)
{
static int index;
ISASerialState *isa = DO_UPCAST(ISASerialState, dev, dev);
SerialState *s = &isa->state;
if (isa->index == -1)
isa->index = index;
if (isa->index >= MAX_SERIAL_PORTS)
return -1;
if (isa->iobase == -1)
isa->iobase = isa_serial_io[isa->index];
if (isa->isairq == -1)
isa->isairq = isa_serial_irq[isa->index];
index++;
s->baudbase = 115200;
isa_init_irq(dev, &s->irq, isa->isairq);
serial_init_core(s);
qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
isa_init_ioport_range(dev, isa->iobase, 8);
return 0;
}
SerialState *serial_init(int base, qemu_irq irq, int baudbase,
CharDriverState *chr)
{
SerialState *s;
s = qemu_mallocz(sizeof(SerialState));
s->irq = irq;
s->baudbase = baudbase;
s->chr = chr;
serial_init_core(s);
vmstate_register(NULL, base, &vmstate_serial, s);
register_ioport_write(base, 8, 1, serial_ioport_write, s);
register_ioport_read(base, 8, 1, serial_ioport_read, s);
return s;
}
static ISADeviceInfo serial_isa_info = {
.qdev.name = "isa-serial",
.qdev.size = sizeof(ISASerialState),
.qdev.vmsd = &vmstate_isa_serial,
.init = serial_isa_initfn,
.qdev.props = (Property[]) {
DEFINE_PROP_UINT32("index", ISASerialState, index, -1),
DEFINE_PROP_HEX32("iobase", ISASerialState, iobase, -1),
DEFINE_PROP_UINT32("irq", ISASerialState, isairq, -1),
DEFINE_PROP_CHR("chardev", ISASerialState, state.chr),
DEFINE_PROP_END_OF_LIST(),
},
};
static void serial_register_devices(void)
{
isa_qdev_register(&serial_isa_info);
}
device_init(serial_register_devices)
To create a device, I need to do:
{
ISADevice *dev;
dev = isa_create("isa-serial");
if (dev == NULL) {
return error;
}
if (qdev_set_uint32(&dev->qdev, "index", index)) {
goto err;
}
if (qdev_set_uint32(&dev->qdev, "iobase", iobase)) {
goto err;
}
if (qdev_set_uint32(&dev->qdev, "irq", irq)) {
goto err;
}
if (qdev_set_chr(&dev->qdev, "chardev", chr)) {
goto err;
}
if (qdev_init(&dev->qdev)) {
goto err;
}
return 0;
err:
qdev_destroy(&dev->qdev);
return -1;
}
This is simply not a reasonable API to use to create devices. There are
two ways we can make this more managable. The first is gobject-style
vararg constructor coupled with a type safe wrapper. So...
ISASerialDevice *isa_serial_device_new(uint32_t index, uint32_t iobase,
uint32_t irq, CharDriverState *chr)
{
return isa_device_create_va("isa-seral", "index", index,
"iobase", iobase, "irq", irq, "chardev", chr, NULL);
}
Now this can be used in a reasonable fashion. However, we can do even
better if we change the way qdev is done. Consider the following:
SerialState *serial_init(int base, qemu_irq irq, int baudbase,
CharDriverState *chr)
{
SerialState *s;
s = qemu_mallocz(sizeof(SerialState));
s->irq = irq;
s->baudbase = baudbase;
s->chr = chr;
serial_init_core(s);
vmstate_register(NULL, base, &vmstate_serial, s);
register_ioport_write(base, 8, 1, serial_ioport_write, s);
register_ioport_read(base, 8, 1, serial_ioport_read, s);
return s;
}
ISASerialDevice *isa_serial_new(uint32_t index, uint32_t iobase,
uint32_t irq, CharDriverState *chr)
{
static int index;
ISADevice *dev = isa_create("isa-serial");
ISASerialState *isa = DO_UPCAST(ISASerialState, dev, dev);
SerialState *s = &isa->state;
if (index == -1)
index = index;
if (index >= MAX_SERIAL_PORTS)
return NULL;
if (iobase == -1)
iobase = isa_serial_io[index];
if (isairq == -1)
isairq = isa_serial_irq[index];
index++;
s->baudbase = 115200;
isa_init_irq(dev, &s->irq, isairq);
serial_init_core(s);
qdev_set_legacy_instance_id(&dev->qdev, isa->iobase, 3);
register_ioport_write(isa->iobase, 8, 1, serial_ioport_write, s);
register_ioport_read(isa->iobase, 8, 1, serial_ioport_read, s);
isa_init_ioport_range(dev, isa->iobase, 8);
return isa;
}
static ISADevice *isa_seral_init_qdev(QemuOpts *opts)
{
uint32_t index = qemu_opt_get_uint32(opts, "index", -1);
uint32_t irq = qemu_opt_get_uint32(opts, "irq", -1);
uint32_t iobase = qemu_opt_get_uint32(opts, "iobase", -1);
CharDriverState *chr = qemu_opt_get_chr(opts, "chardev");
return isa_serial_new(index, irq, iobase, chr);
}
static void serial_register_devices(void)
{
isa_qdev_register("isa-serial", isa_serial_init_qdev);
}
device_init(serial_register_devices)
The advantage of this model is that we can totally ignore the factory
interface for devices that we don't care about exposing to the user. So
the isa_seral_init_qdev part is totally optional.
Regards,
Anthony Liguori
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
