| Message ID | 1469523803-12194-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On (Tue) 26 Jul 2016 [17:03:23], Cao jin wrote: > My previous commit 2e2aa316 removed internal flag msi_in_use, which > exists in vmstate, use VMSTATE_UNUSED for migration compatibility. > > Reported-by: Amit Shah <amit.shah@redhat.com> > Suggested-by: Amit Shah <amit.shah@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Marcel Apfelbaum <marcel@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Amit Shah <amit.shah@redhat.com> > Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> Reviewed-by: Amit Shah <amit.shah@redhat.com> Amit
Hi On 08/01/2016 11:29 PM, Amit Shah wrote: > On (Mon) 01 Aug 2016 [10:16:50], Paolo Bonzini wrote: >>> @@ -1370,7 +1370,7 @@ static const VMStateDescription vmstate_mptsas = { >>> .post_load = mptsas_post_load, >>> .fields = (VMStateField[]) { >>> VMSTATE_PCI_DEVICE(dev, MPTSASState), >>> - >>> + VMSTATE_UNUSED(sizeof(bool)), /* Was msi_in_use */ >> >> This needs to be "1", not sizeof(bool), because vmstate_info_bool writes >> a single byte. I'll fix this and queue the patch (removing Amit's >> reviewed-by since it's effectively a different change). > > Eeks, yes. > > This patch was merged in the meantime, so Cao Jin, please post a > revert and a fix, thanks! > > Amit > Before I send the fix, I did a quick test on linux as following: #include <stdio.h> #include <stdbool.h> int main() { printf("bool size = %d\n", sizeof(bool)); } then: ./a.out bool size = 1 and there is mptsas.c #include "qemu/osdep.h", osdep.h #include <stdbool.h> So, am I missing something?
> Before I send the fix, I did a quick test on linux as following: > > #include <stdio.h> > #include <stdbool.h> > > int main() > { > printf("bool size = %d\n", sizeof(bool)); > } > > > then: > ./a.out > bool size = 1 > > and there is mptsas.c #include "qemu/osdep.h", osdep.h #include <stdbool.h> > > So, am I missing something? You are missing that the patch is: 1) not portable, because there's no guarantee that sizeof(bool)==1; 2) wrong, because using sizeof within VMSTATE_UNUSED would suggest a dependency of the migration stream on the host---and such a dependency should not be there at all. In fact, the patch is also wrong because old versions of QEMU _do_ need msi_in_use so you need to either generate it or bump the migration version. I'm sending a patch now. Paolo
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c index c1a0649..0ed24d1 100644 --- a/hw/scsi/mptsas.c +++ b/hw/scsi/mptsas.c @@ -1370,7 +1370,7 @@ static const VMStateDescription vmstate_mptsas = { .post_load = mptsas_post_load, .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(dev, MPTSASState), - + VMSTATE_UNUSED(sizeof(bool)), /* Was msi_in_use */ VMSTATE_UINT32(state, MPTSASState), VMSTATE_UINT8(who_init, MPTSASState), VMSTATE_UINT8(doorbell_state, MPTSASState),
My previous commit 2e2aa316 removed internal flag msi_in_use, which exists in vmstate, use VMSTATE_UNUSED for migration compatibility. Reported-by: Amit Shah <amit.shah@redhat.com> Suggested-by: Amit Shah <amit.shah@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Marcel Apfelbaum <marcel@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Amit Shah <amit.shah@redhat.com> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com> --- hw/scsi/mptsas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)