| Message ID | 20190725163710.11703-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Typecheck VMSTATE VARRAY macros and fix bug found | expand |
* Peter Maydell (peter.maydell@linaro.org) wrote: > gamepad_state::buttons is a pointer to an array of structs, > not an array of structs, so should be declared in the vmstate > with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we > corrupt memory on incoming migration. > > We bump the vmstate version field as the easiest way to > deal with the migration break, since migration wouldn't have > worked reliably before anyway. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> OK, it would be great to change num_buttons to uint32_t and make that a UINT32 at some point; it's hard to press negative buttons. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/input/stellaris_input.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c > index 20c87d86f40..3a666d61d47 100644 > --- a/hw/input/stellaris_input.c > +++ b/hw/input/stellaris_input.c > @@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = { > > static const VMStateDescription vmstate_stellaris_gamepad = { > .name = "stellaris_gamepad", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_INT32(extension, gamepad_state), > - VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0, > - vmstate_stellaris_button, gamepad_button), > + VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state, > + num_buttons, > + vmstate_stellaris_button, > + gamepad_button), > VMSTATE_END_OF_LIST() > } > }; > -- > 2.20.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote: > * Peter Maydell (peter.maydell@linaro.org) wrote: >> gamepad_state::buttons is a pointer to an array of structs, >> not an array of structs, so should be declared in the vmstate >> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we >> corrupt memory on incoming migration. >> >> We bump the vmstate version field as the easiest way to >> deal with the migration break, since migration wouldn't have >> worked reliably before anyway. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > OK, it would be great to change num_buttons to uint32_t and make that a > UINT32 at some point; it's hard to press negative buttons. Since the version is incremented, now seems to be a good time. > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- >> hw/input/stellaris_input.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c >> index 20c87d86f40..3a666d61d47 100644 >> --- a/hw/input/stellaris_input.c >> +++ b/hw/input/stellaris_input.c >> @@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = { >> >> static const VMStateDescription vmstate_stellaris_gamepad = { >> .name = "stellaris_gamepad", >> - .version_id = 1, >> - .minimum_version_id = 1, >> + .version_id = 2, >> + .minimum_version_id = 2, >> .fields = (VMStateField[]) { >> VMSTATE_INT32(extension, gamepad_state), >> - VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0, >> - vmstate_stellaris_button, gamepad_button), >> + VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state, >> + num_buttons, >> + vmstate_stellaris_button, >> + gamepad_button), >> VMSTATE_END_OF_LIST() >> } >> }; >> -- >> 2.20.1 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > gamepad_state::buttons is a pointer to an array of structs, > > not an array of structs, so should be declared in the vmstate > > with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we > > corrupt memory on incoming migration. > > > > We bump the vmstate version field as the easiest way to > > deal with the migration break, since migration wouldn't have > > worked reliably before anyway. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > OK, it would be great to change num_buttons to uint32_t and make that a > UINT32 at some point; it's hard to press negative buttons. Is there much benefit though? As an aside, I'm surprised also the macro doesn't complain that we said the num_buttons field is int32 but it's really "int"... arguably a different kind of missing type check. thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > > gamepad_state::buttons is a pointer to an array of structs, > > > not an array of structs, so should be declared in the vmstate > > > with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we > > > corrupt memory on incoming migration. > > > > > > We bump the vmstate version field as the easiest way to > > > deal with the migration break, since migration wouldn't have > > > worked reliably before anyway. > > > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > OK, it would be great to change num_buttons to uint32_t and make that a > > UINT32 at some point; it's hard to press negative buttons. > > Is there much benefit though? Not much and it's not urgent > As an aside, I'm surprised also the macro doesn't complain > that we said the num_buttons field is int32 but it's really "int"... > arguably a different kind of missing type check. I was just concerned what would happen if your migration stream had a negative value in. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 7/25/19 7:59 PM, Peter Maydell wrote: > On Thu, 25 Jul 2019 at 18:02, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: >> >> * Peter Maydell (peter.maydell@linaro.org) wrote: >>> gamepad_state::buttons is a pointer to an array of structs, >>> not an array of structs, so should be declared in the vmstate >>> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we >>> corrupt memory on incoming migration. >>> >>> We bump the vmstate version field as the easiest way to >>> deal with the migration break, since migration wouldn't have >>> worked reliably before anyway. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> > As an aside, I'm surprised also the macro doesn't complain > that we said the num_buttons field is int32 but it's really "int"... > arguably a different kind of missing type check. We would need to compile on a host with int size not being 32bit to catch this kind of problem I think. -- Damien
On Fri, 26 Jul 2019 at 09:25, Damien Hedde <damien.hedde@greensocs.com> wrote: > On 7/25/19 7:59 PM, Peter Maydell wrote: > > As an aside, I'm surprised also the macro doesn't complain > > that we said the num_buttons field is int32 but it's really "int"... > > arguably a different kind of missing type check. > > We would need to compile on a host with int size not being 32bit to > catch this kind of problem I think. I was under the impression we did catch attempts to use "int" with the VMSTATE_INT32() macro, but we do apply the type-checking magic to the 'value' fields in the VARRAY macros, so I guess I'm misremembering. thanks -- PMM
On Thu, 25 Jul 2019 at 18:40, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > >> gamepad_state::buttons is a pointer to an array of structs, > >> not an array of structs, so should be declared in the vmstate > >> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we > >> corrupt memory on incoming migration. > >> > >> We bump the vmstate version field as the easiest way to > >> deal with the migration break, since migration wouldn't have > >> worked reliably before anyway. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > OK, it would be great to change num_buttons to uint32_t and make that a > > UINT32 at some point; it's hard to press negative buttons. > > Since the version is incremented, now seems to be a good time. ...except this patch is for 4.1 and we've already done rc2, so it's not really an ideal time to put in code cleanups... thanks -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Thu, 25 Jul 2019 at 18:40, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > On 7/25/19 7:02 PM, Dr. David Alan Gilbert wrote: > > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > >> gamepad_state::buttons is a pointer to an array of structs, > > >> not an array of structs, so should be declared in the vmstate > > >> with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we > > >> corrupt memory on incoming migration. > > >> > > >> We bump the vmstate version field as the easiest way to > > >> deal with the migration break, since migration wouldn't have > > >> worked reliably before anyway. > > >> > > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > > > > OK, it would be great to change num_buttons to uint32_t and make that a > > > UINT32 at some point; it's hard to press negative buttons. > > > > Since the version is incremented, now seems to be a good time. > > ...except this patch is for 4.1 and we've already done rc2, > so it's not really an ideal time to put in code cleanups... Don't worry; you can always change the int to a uint later without bumping the version again. Unless someone somewhere has a device with -ve buttons it'll be fine. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Fri, 26 Jul 2019 at 10:59, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > Don't worry; you can always change the int to a uint later without > bumping the version again. Unless someone somewhere has a device with > -ve buttons it'll be fine. The number of buttons is a constant set up when the device is created (it would be a QOM property if this device had been converted to QOM; as it is it's an argument to the stellaris_gamepad_init() function) and the num_buttons field itself is not migrated. As it happens the one caller of this function passes the constant value "5"... thanks -- PMM
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c index 20c87d86f40..3a666d61d47 100644 --- a/hw/input/stellaris_input.c +++ b/hw/input/stellaris_input.c @@ -60,12 +60,14 @@ static const VMStateDescription vmstate_stellaris_button = { static const VMStateDescription vmstate_stellaris_gamepad = { .name = "stellaris_gamepad", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_INT32(extension, gamepad_state), - VMSTATE_STRUCT_VARRAY_INT32(buttons, gamepad_state, num_buttons, 0, - vmstate_stellaris_button, gamepad_button), + VMSTATE_STRUCT_VARRAY_POINTER_INT32(buttons, gamepad_state, + num_buttons, + vmstate_stellaris_button, + gamepad_button), VMSTATE_END_OF_LIST() } };
gamepad_state::buttons is a pointer to an array of structs, not an array of structs, so should be declared in the vmstate with VMSTATE_STRUCT_VARRAY_POINTER_INT32; otherwise we corrupt memory on incoming migration. We bump the vmstate version field as the easiest way to deal with the migration break, since migration wouldn't have worked reliably before anyway. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/input/stellaris_input.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) -- 2.20.1