| Message ID | 1454073536-19963-1-git-send-email-dgilbert@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 29 Jan 2016 13:18:56 +0000 "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > I misunderstood the vmstate macro definition when I reworked the > virtio .get/.put. > The VMSTATE_STRUCT_VARRAY_KNOWN, was described as being for "a > variable length array (i.e. _type *_field) but we know the > length". However it actually specified operation for arrays embedded in > the struct (i.e. _type _field[]) since it lacked the VMS_POINTER > flag. This caused offset calculation to be completely off, examining and > potentially sending random data instead of the VirtQueue content. > > Replace the otherwise unused VMSTATE_STRUCT_VARRAY_KNOWN with a > VMSTATE_STRUCT_VARRAY_POINTER_KNOWN that includes the VMS_POINTER flag > (so now actually doing what it advertises) and use it in the virtio > migration code. > > Fixes and description as per Sascha's suggestions/debug. > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reported-by: Sascha Silbe <silbe@linux.vnet.ibm.com> > Tested-By: Sascha Silbe <silbe@linux.vnet.ibm.com> > Reviewed-By: Sascha Silbe <silbe@linux.vnet.ibm.com> > > Fixes: 50e5ae4dc3e4f21e874512f9e87b93b5472d26e0 > Fixes: 2cf0148674430b6693c60d42b7eef721bfa9509f > --- > hw/virtio/virtio.c | 8 ++++---- > include/migration/vmstate.h | 18 +++++++++--------- > 2 files changed, 13 insertions(+), 13 deletions(-) Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>
On 29 January 2016 at 13:18, Dr. David Alan Gilbert (git) <dgilbert@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > I misunderstood the vmstate macro definition when I reworked the > virtio .get/.put. > The VMSTATE_STRUCT_VARRAY_KNOWN, was described as being for "a > variable length array (i.e. _type *_field) but we know the > length". However it actually specified operation for arrays embedded in > the struct (i.e. _type _field[]) since it lacked the VMS_POINTER > flag. This caused offset calculation to be completely off, examining and > potentially sending random data instead of the VirtQueue content. > > Replace the otherwise unused VMSTATE_STRUCT_VARRAY_KNOWN with a > VMSTATE_STRUCT_VARRAY_POINTER_KNOWN that includes the VMS_POINTER flag > (so now actually doing what it advertises) and use it in the virtio > migration code. Yeah, these macro names are a bit of a mess. I had an idea ages back about autogenerating them all as an orthogonal cross product of the different kinds of thing (and filling in some of the random gaps in coverage as a side effect), but I never really got anywhere with it. thanks -- PMM
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index bd6b4df..41a8a8a 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1143,8 +1143,8 @@ static const VMStateDescription vmstate_virtio_virtqueues = { .minimum_version_id = 1, .needed = &virtio_virtqueue_needed, .fields = (VMStateField[]) { - VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, - 0, vmstate_virtqueue, VirtQueue), + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_virtqueue, VirtQueue), VMSTATE_END_OF_LIST() } }; @@ -1165,8 +1165,8 @@ static const VMStateDescription vmstate_virtio_ringsize = { .minimum_version_id = 1, .needed = &virtio_ringsize_needed, .fields = (VMStateField[]) { - VMSTATE_STRUCT_VARRAY_KNOWN(vq, struct VirtIODevice, VIRTIO_QUEUE_MAX, - 0, vmstate_ringsize, VirtQueue), + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_ringsize, VirtQueue), VMSTATE_END_OF_LIST() } }; diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index a4b81bb..7246f29 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -386,26 +386,26 @@ extern const VMStateInfo vmstate_info_bitmap; .offset = vmstate_offset_array(_state, _field, _type, _num),\ } -/* a variable length array (i.e. _type *_field) but we know the - * length - */ -#define VMSTATE_STRUCT_VARRAY_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ +#define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, _vmsd, _type) { \ .name = (stringify(_field)), \ - .num = (_num), \ + .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \ .version_id = (_version), \ .vmsd = &(_vmsd), \ .size = sizeof(_type), \ - .flags = VMS_STRUCT|VMS_ARRAY, \ + .flags = VMS_STRUCT|VMS_VARRAY_UINT8, \ .offset = offsetof(_state, _field), \ } -#define VMSTATE_STRUCT_VARRAY_UINT8(_field, _state, _field_num, _version, _vmsd, _type) { \ +/* a variable length array (i.e. _type *_field) but we know the + * length + */ +#define VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(_field, _state, _num, _version, _vmsd, _type) { \ .name = (stringify(_field)), \ - .num_offset = vmstate_offset_value(_state, _field_num, uint8_t), \ + .num = (_num), \ .version_id = (_version), \ .vmsd = &(_vmsd), \ .size = sizeof(_type), \ - .flags = VMS_STRUCT|VMS_VARRAY_UINT8, \ + .flags = VMS_STRUCT|VMS_ARRAY|VMS_POINTER, \ .offset = offsetof(_state, _field), \ }