* Halil Pasic (pa...@linux.vnet.ibm.com) wrote:
>
>
> On 01/31/2017 09:00 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pa...@linux.vnet.ibm.com) 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.
>
> Thanks for the reply! Let us try to figure out what to do together...
> >
> > 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?
>
> I'm going to explain my view on field->start, but first let us have a look
> which macros are using it:
>
> $ pcregrep -Mi '#define VMSTATE_[^{]*{[^}]*\.start[^}]*}'
> include/migration/vmstate.h
nice :-)
> #define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start,
> _field_size, _multiply) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .size = (_multiply), \
> .info = &vmstate_info_buffer, \
> .flags = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY, \
> .offset = offsetof(_state, _field), \
> .start = (_start), \
> }
> #define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size)
> { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, int32_t),\
> .info = &vmstate_info_buffer, \
> .flags = VMS_VBUFFER|VMS_POINTER, \
> .offset = offsetof(_state, _field), \
> .start = (_start), \
> }
> #define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start,
> _field_size) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .info = &vmstate_info_buffer, \
> .flags = VMS_VBUFFER|VMS_POINTER, \
> .offset = offsetof(_state, _field), \
> .start = (_start), \
> }
> #define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, _start,
> _field_size) { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .field_exists = (_test), \
> .size_offset = vmstate_offset_value(_state, _field_size, uint32_t),\
> .info = &vmstate_info_buffer, \
> .flags = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC, \
> .offset = offsetof(_state, _field), \
> .start = (_start), \
> }
> #define VMSTATE_QTAILQ_V(_field, _state, _version, _vmsd, _type, _next) \
> { \
> .name = (stringify(_field)), \
> .version_id = (_version), \
> .vmsd = &(_vmsd), \
> .size = sizeof(_type), \
> .info = &vmstate_info_qtailq, \
> .offset = offsetof(_state, _field), \
> .start = offsetof(_type, _next), \
> }
>
> We both agree that VMSTATE_VBUFFER_ALLOC_UINT32 is wrong with start != 0,
> and that VMSTATE_QTAILQ_V needs the data member. But I think
> VMSTATE_QTAILQ_V is not using the start handling from vmstate_base_addr,
> (because it has no VMS_POINTER flag set).
Yes, I think I agree.
> I think the not allocating versions of VMSTATE_VBUFFER are also fine as
> is, but if someone had the bright idea to make an allocating version,
> it's very straightforward to create something broken.
Yes.
> So my original train of thought was, because nobody uses VMSTATE_VBUFFER
> with start != 0 let us get rid of the extra complexity, and eventually
> reintroduce it when it's needed. This feature is around for quite some
> time now, but nobody uses it, and I do not think it is very likely we
> will need it in the future.
>
>
> > If we can't remove the start variable then it's probably not removing
> > the start parameter everywhere.
>
> Yes we can. We just can't remove the start data member form VMStateField
> because the QTAILQ stuff is now using it.
> >
> > 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.
>
> I also think it makes sense, but as written above, it is never used.
>
> I see 3 options regarding how to proceed:
>
> 1) Remove the start parameter from the VMSTATE_VBUFFER* macros, and
> the start handling from vmstate_base_addr (basically the same I
> did here, with the difference that VMStateField.start remains).
> 2) Proclaim that .start && (.flags & VMSTATE_ALLOC) == false is an
> invariant of QEMU, and assert when we would allocate. Remove the parameter
> form VMSTATRE_VBUFFER_ALLOC.
> 3) Make .start work properly for dynamically allocated buffers.
>
> I prefer option 1). What is your preference?
Yes OK; that probably makes sense; I slightly preferred (2) but I think
you convinced me :-)
Dave
> Halil
>
> >
> > Dave
> >
> >> Regards,
> >> Halil
> >>
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
