| Message ID | 1393347170-28502-2-git-send-email-a.rigo@virtualopensystems.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Feb 25, 2014 at 05:52:47PM +0100, Alvise Rigo wrote: > The method is not behaving in the way it's supposed to. It should return > the new value only if it's less than the actual one. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> > --- > vmstate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/vmstate.c b/vmstate.c > index 284b080..038b274 100644 > --- a/vmstate.c > +++ b/vmstate.c > @@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = { > > static int get_int32_le(QEMUFile *f, void *pv, size_t size) > { > - int32_t *old = pv; > - int32_t new; > - qemu_get_sbe32s(f, &new); > + int32_t old = *(int32_t *)pv; > + int32_t *new = pv; > + qemu_get_sbe32s(f, new); You are now changing the value in *(int32_t*)pv on every call, instead of simply ensuring the value is less than the current value. This doesn't seem to be the intended behavior of vmstate_info_int32_le/VMSTATE_INT32_LE. > > - if (*old <= new) { > + if (*new <= old) { > return 0; > } > return -EINVAL; > -- > 1.8.3.2 >
On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote: > The method is not behaving in the way it's supposed to. It should return > the new value only if it's less than the actual one. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> This clashes with David Gilbert's patch to this function: http://patchwork.ozlabs.org/patch/319717/ which I suspect is fixing the same bug you're trying to address here. thanks -- PMM
Alvise Rigo <a.rigo@virtualopensystems.com> wrote: > The method is not behaving in the way it's supposed to. It should return > the new value only if it's less than the actual one. > > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> See David patch of this function. There were a bug, we were doing the wrong comparison. But we expect not to chang the local value. We just want the the one that cames is less or equal that the current value (think of an array size, it is a bad idea to try to read a bigger array into a smaller one). BTW, did you find this bug by testing or by code inspection? thanks, Juan. > --- > vmstate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/vmstate.c b/vmstate.c > index 284b080..038b274 100644 > --- a/vmstate.c > +++ b/vmstate.c > @@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = { > > static int get_int32_le(QEMUFile *f, void *pv, size_t size) > { > - int32_t *old = pv; > - int32_t new; > - qemu_get_sbe32s(f, &new); > + int32_t old = *(int32_t *)pv; > + int32_t *new = pv; > + qemu_get_sbe32s(f, new); > > - if (*old <= new) { > + if (*new <= old) { > return 0; > } > return -EINVAL;
On 25 February 2014 18:52, Juan Quintela <quintela@redhat.com> wrote:
> BTW, did you find this bug by testing or by code inspection?
Trying a KVM<->TCG migration of an ARM guest will likely
hit the bug (because the number of coprocessor registers
on either end differs).
thanks
-- PMM
diff --git a/vmstate.c b/vmstate.c index 284b080..038b274 100644 --- a/vmstate.c +++ b/vmstate.c @@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = { static int get_int32_le(QEMUFile *f, void *pv, size_t size) { - int32_t *old = pv; - int32_t new; - qemu_get_sbe32s(f, &new); + int32_t old = *(int32_t *)pv; + int32_t *new = pv; + qemu_get_sbe32s(f, new); - if (*old <= new) { + if (*new <= old) { return 0; } return -EINVAL;
The method is not behaving in the way it's supposed to. It should return the new value only if it's less than the actual one. Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com> --- vmstate.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)