| Message ID | 20231020090731.28701-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Check for duplicates on vmstate_register() | expand |
On 20/10/2023 11.07, Juan Quintela wrote: > Otherwise qom-test fails. > > ok 4 /i386/qom/x-remote > qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 > Broken pipe > ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) > Aborted (core dumped) > $ > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Signed-off-by: Juan Quintela <quintela@redhat.com> > --- > hw/ide/isa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ide/isa.c b/hw/ide/isa.c > index 95053e026f..ea60c08116 100644 > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) > ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); > ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); > ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); > - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); > + vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); > ide_bus_register_restart_cb(&s->bus); > } Would it make sense to use another unique ID of the device instead? E.g.: diff a/hw/ide/isa.c b/hw/ide/isa.c --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); + vmstate_register(VMSTATE_IF(dev), + object_property_get_int(OBJECT(dev), "irq", &error_abort), + &vmstate_ide_isa, s); ide_bus_register_restart_cb(&s->bus); } Thomas
Thomas Huth <thuth@redhat.com> wrote: > On 20/10/2023 11.07, Juan Quintela wrote: >> Otherwise qom-test fails. >> ok 4 /i386/qom/x-remote >> qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 >> Broken pipe >> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) >> Aborted (core dumped) >> $ >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> --- >> hw/ide/isa.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/hw/ide/isa.c b/hw/ide/isa.c >> index 95053e026f..ea60c08116 100644 >> --- a/hw/ide/isa.c >> +++ b/hw/ide/isa.c >> @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) >> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >> - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >> + vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); >> ide_bus_register_restart_cb(&s->bus); >> } > > Would it make sense to use another unique ID of the device instead? E.g.: > > diff a/hw/ide/isa.c b/hw/ide/isa.c > --- a/hw/ide/isa.c > +++ b/hw/ide/isa.c > @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) > ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); > ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); > ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); > - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); > + vmstate_register(VMSTATE_IF(dev), > + object_property_get_int(OBJECT(dev), "irq", &error_abort), > + &vmstate_ide_isa, s); > ide_bus_register_restart_cb(&s->bus); > } > Thomas Ide is not my part of expertise. But anything that is different for each instantance is going to be good for me. The whole point of this series is to be able to test that there are no duplicates. Duplicates are one error when we do real migration. How we reach the goal of no duplicates doesn't matter to me. Later, Juan.
On 20/10/2023 21.42, Juan Quintela wrote: > Thomas Huth <thuth@redhat.com> wrote: >> On 20/10/2023 11.07, Juan Quintela wrote: >>> Otherwise qom-test fails. >>> ok 4 /i386/qom/x-remote >>> qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 >>> Broken pipe >>> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) >>> Aborted (core dumped) >>> $ >>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> --- >>> hw/ide/isa.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> diff --git a/hw/ide/isa.c b/hw/ide/isa.c >>> index 95053e026f..ea60c08116 100644 >>> --- a/hw/ide/isa.c >>> +++ b/hw/ide/isa.c >>> @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) >>> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >>> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >>> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >>> - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >>> + vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); >>> ide_bus_register_restart_cb(&s->bus); >>> } >> >> Would it make sense to use another unique ID of the device instead? E.g.: >> >> diff a/hw/ide/isa.c b/hw/ide/isa.c >> --- a/hw/ide/isa.c >> +++ b/hw/ide/isa.c >> @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) >> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >> - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >> + vmstate_register(VMSTATE_IF(dev), >> + object_property_get_int(OBJECT(dev), "irq", &error_abort), >> + &vmstate_ide_isa, s); >> ide_bus_register_restart_cb(&s->bus); >> } >> Thomas > > Ide is not my part of expertise. > But anything that is different for each instantance is going to be good > for me. It's not really my turf either ... ok, so unless the IDE maintainer speaks up, I think it's maybe best if you continue with your "_any" patch. Thomas
Thomas Huth <thuth@redhat.com> wrote: > On 20/10/2023 21.42, Juan Quintela wrote: >> Thomas Huth <thuth@redhat.com> wrote: >>> On 20/10/2023 11.07, Juan Quintela wrote: >>>> Otherwise qom-test fails. >>>> ok 4 /i386/qom/x-remote >>>> qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0 >>>> Broken pipe >>>> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0) >>>> Aborted (core dumped) >>>> $ >>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> >>>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>>> --- >>>> hw/ide/isa.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> diff --git a/hw/ide/isa.c b/hw/ide/isa.c >>>> index 95053e026f..ea60c08116 100644 >>>> --- a/hw/ide/isa.c >>>> +++ b/hw/ide/isa.c >>>> @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) >>>> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >>>> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >>>> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >>>> - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >>>> + vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); >>>> ide_bus_register_restart_cb(&s->bus); >>>> } >>> >>> Would it make sense to use another unique ID of the device instead? E.g.: >>> >>> diff a/hw/ide/isa.c b/hw/ide/isa.c >>> --- a/hw/ide/isa.c >>> +++ b/hw/ide/isa.c >>> @@ -73,7 +73,9 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) >>> ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); >>> ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); >>> ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); >>> - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); >>> + vmstate_register(VMSTATE_IF(dev), >>> + object_property_get_int(OBJECT(dev), "irq", &error_abort), >>> + &vmstate_ide_isa, s); >>> ide_bus_register_restart_cb(&s->bus); >>> } >>> Thomas >> Ide is not my part of expertise. >> But anything that is different for each instantance is going to be good >> for me. > > It's not really my turf either ... ok, so unless the IDE maintainer > speaks up, I think it's maybe best if you continue with your "_any" > patch. Ide maintainer can do your change if he likes it. It is outside of my understanding to accept it or not (and furthermore, it breaks migration if you have only one device, with more than one it is already broken). Later, Juan.
diff --git a/hw/ide/isa.c b/hw/ide/isa.c index 95053e026f..ea60c08116 100644 --- a/hw/ide/isa.c +++ b/hw/ide/isa.c @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp) ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2); ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2); ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum)); - vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s); + vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s); ide_bus_register_restart_cb(&s->bus); }