[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.star
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] migration: drop unused VMStateField.start |
Date: |
Tue, 31 Jan 2017 20:00:17 +0000 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Halil Pasic (address@hidden) wrote:
>
>
> On 10/20/2016 03:25 PM, Halil Pasic wrote:
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index fc29acf..8767e40 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -66,10 +66,10 @@ static void *vmstate_base_addr(void *opaque,
> > VMStateField *field, bool alloc)
> > }
> > }
> > if (size) {
> > - *((void **)base_addr + field->start) = g_malloc(size);
> > + *(void **)base_addr = g_malloc(size);
> > }
> > }
> > - base_addr = *(void **)base_addr + field->start;
> > + base_addr = *(void **)base_addr;
> > }
> >
> > return base_addr;
> Hi!
>
> It is been a while, and IMHO this is still broken, and the
> VMSTATE_VBUFFER* macros are still only used with the start argument
> being zero.
>
> What changed is that with commit 94869d5c ("migration: migrate QTAILQ")
> from Jan 19 we have code actually using VMStateDecription.start -- but
> for something different (IMHO), as allocation is done by get_qtailq and
> not by vmstate_base_addr (as in case of VMSTATE_VBUFFER_ALLOC_UINT32).
> Thus I would need to update the commit message and keep the start field
> at least.
>
> But before I do so, I would like to ask the maintainers if there is
> interest in a change like this?
I think it's certainly worth fixing VMSTATE_BUFFER_ALLOC*; if it can't
use the start field properly then it should have the start parameter
removed.
I'll admit I can't remember the details of why field->start as an offset
didn't work; are the other macros that take a start value correct?
If we can't remove the start variable then it's probably not removing
the start parameter everywhere.
That alloc case looks the most unusual in that vmstate_base_addr()
function; I *think* the non-alloc case makes sense
(i.e. follow a pointer and then use an offset from that pointer?)
even if no one currently uses it.
Dave
> Regards,
> Halil
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK