Re: [Qemu-devel] Memory API: handling unassigned physical memory

[Anthony Liguori]

On 05/01/2012 09:20 AM, Peter Maydell wrote:
> On 1 May 2012 15:06, Anthony Liguori<address@hidden>  wrote:
> > Do you mean:
> >
> > -    qdev_connect_gpio_out(&dev->qdev, 0, irq_set[2]);
> > +    pin_connect_qemu_irq(&s->int_out[0], irq_set[2]);
> >
> > There are three ways to do this:
> >
> > 1) pin_connect_qemu_irq(i8259_get_int_out(s), irq_set[2]);
>
> No good unless you're autogenerating all those accessor functions.
>
> > 2) pin_connect_qemu_irq(&s->int_out[0], irq_set[2]);
>
> Exposes the whole of the struct (including all the private
> memmbers) to anything that wants to use it. As you say
> later on in the email, this requires splitting everything up
> into two structs, one for "publicly accessible" and one for
> "private implementation"...which your series doesn't seem
> to do at the moment.
>
> > 3) pin_connect_qemu_irq(PIN(object_get_child(s, "int_out[0]")), irq_set[2]);
>
> This is a bit verbose. Something more like
>   pin_connect(s, "int_out[0]", self, "int_set[2]");

This is incorrect, it would have to be:

Error *err = NULL;

if (pin_connect(s, "in_out[0]", self, "int_set[2]", &err)) {
   error_propagate(errp, err);
   return;
}

> would be a bit less longwinded. Or even
>   connect(TYPE_PIN, s, "int_out[0]", self, "int_set[2]");

Not checking for failure is not an option.

> (No, this doesn't do compile time type checking. I don't
> think that's a problem particularly, or at least not enough
> of one to justify not doing it this way. The object model we
> have is dynamic and does things at runtime...)

Correctness is more important to me than brevity.

And really, we should focus on killing things like i8259_init().  These types of 
functions should go away in favor of proper modeling of SoCs/SuperIO chips.

Regards,

Anthony Liguori

> -- PMM