Am 19.05.2014 03:52, schrieb Peter Crosthwaite:
> On Fri, May 16, 2014 at 11:56 AM, Peter Crosthwaite
> <peter.crosthwa...@xilinx.com> wrote:
>> From: Andreas Färber <afaer...@suse.de>
>>
>> As a prequel to any big Pin refactoring plans, do an in-place conversion
>> of qemu_irq to an Object, so that we can reference it in link<> properties.
>>
>> Signed-off-by: Andreas Färber <afaer...@suse.de>
(Note that you forgot to sign off here.)
>> ---
>>
>> hw/core/irq.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>> include/hw/irq.h | 2 ++
>> 2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/core/irq.c b/hw/core/irq.c
>> index 03c8cb3..0bcd27b 100644
>> --- a/hw/core/irq.c
>> +++ b/hw/core/irq.c
>> @@ -23,8 +23,13 @@
>> */
>> #include "qemu-common.h"
>> #include "hw/irq.h"
>> +#include "qom/object.h"
>> +
>> +#define IRQ(obj) OBJECT_CHECK(struct IRQState, (obj), TYPE_IRQ)
>>
>> struct IRQState {
>> + Object parent_obj;
>> +
>> qemu_irq_handler handler;
>> void *opaque;
>> int n;
>> @@ -38,6 +43,14 @@ void qemu_set_irq(qemu_irq irq, int level)
>> irq->handler(irq->opaque, irq->n, level);
>> }
>>
>> +static void irq_nonfirst_free(void *obj)
>> +{
>> + struct IRQState *s = obj;
>> +
>> + /* Unreference the first IRQ in this array */
>> + object_unref(OBJECT(s - s->n));
>> +}
>> +
>> qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler
>> handler,
>> void *opaque, int n)
>> {
>> @@ -51,11 +64,23 @@ qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old,
>> qemu_irq_handler handler,
>> s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
>> p = old ? g_renew(struct IRQState, s[0], n + n_old) :
>> g_new(struct IRQState, n);
>
> So using g_renew on the actual object storage is very fragile, as it
> means you cannot reliably take pointers to the objects. I think
> however this whole issue could be simplified by de-arrayifying the
> whole thing, and treating IRQs individually (interdiff):
>
> qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler
> handler,
> void *opaque, int n)
> {
> qemu_irq *s;
> - struct IRQState *p;
> int i;
>
> if (!old) {
> n_old = 0;
> }
> s = old ? g_renew(qemu_irq, old, n + n_old) : g_new(qemu_irq, n);
> - p = old ? g_renew(struct IRQState, s[0], n + n_old) :
> - g_new(struct IRQState, n);
> - memset(p + n_old, 0, n * sizeof(*p));
> for (i = 0; i < n + n_old; i++) {
> if (i >= n_old) {
> - Object *obj;
> -
> - object_initialize(p, sizeof(*p), TYPE_IRQ);
> - p->handler = handler;
> - p->opaque = opaque;
> - p->n = i;
> - obj = OBJECT(p);
> - /* Let the first IRQ clean them all up */
> - if (unlikely(i == 0)) {
> - obj->free = g_free;
> - } else {
> - object_ref(OBJECT(s[0]));
> - obj->free = irq_nonfirst_free;
> - }
> + s[i] = qemu_allocate_irq(handler, opaque, i);
> }
> - s[i] = p;
> - p++;
> }
> return s;
>
> The system for freeing may need some thought, and I wonder if their
> are API clients out there assuming the IRQ storage elements are
> contiguous (if there are they have too much internal knowledge and
> need to be fixed).
>
> But with this change these objects are now usable with links and canon
> paths etc.
>
> Will roll into V2 of this patch.
Negative!
I was well aware of the two g_renew()s, one of which affects the
objects. However I figured, as long as no one has a pointer to those
objects it should just continue work (otherwise the pre-QOM pointers
would've broken, too -> separate issue) - and so far I haven't found a
test case that doesn't work as is.
So while I agree that we should refactor this, your series does iirc not
include my genuine qemu_irq* fixes either, and I reported at least two
callers of qemu_free_irqs() remaining, in serial-pci.c among others, so
your diff above is not yet okay - it would leak any non-first IRQState.
Which is why I do these odd object_{ref,unref}() tricks -
qemu_free_irqs() cannot tell how many IRQs there are to be freed. And
while your use of qemu_allocate_irq() gives them the normal oc->free =
g_free for freeing them individually, you do not unref them anywhere, so
it will never happen.
Instead of squashing this into my patch I suggest you build upon master
plus my first two cleanup patches and get rid of the remaining
qemu_free_irqs()s altogether, then we can much more sanely refactor this
code and get it in shortly.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
