Re: [Qemu-devel] [PULL 42/43] piix4: add acpi pci hotplug support

[Anthony Liguori <anthony@codemonkey.ws>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 15/10/2013 16:35, Michael S. Tsirkin ha scritto:
>> On Tue, Oct 15, 2013 at 04:31:31PM +0200, Paolo Bonzini wrote:
>>> Il 14/10/2013 17:01, Michael S. Tsirkin ha scritto:
>>>> -        VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
>>>> -                       struct pci_status),
>>>> +        VMSTATE_STRUCT_TEST(pci0_status, PIIX4PMState,
>>>> +                            vmstate_test_no_use_acpi_pci_hotplug,
>>>> +                            2, vmstate_pci_status,
>>>> +                            struct pci_status),
>>>
>>> There's no reason to remove this from the stream when a new machine type
>>> is in use.  You'll just send out zeroes.
>> 
>> Seemed cleaner not to.
>
> It certainly would be if we had a self-descriptive migration stream
> format.

Yes, removing tests is always a good thing.

But I think subsections should always be used when they can.  We should
not break compatibility (even if we technical don't guarantee it) unless
we absolutely have to.

Regards,

Anthony Liguori

>
> However, what we have is "send bytes, parse them on the destination,
> hope the format matches".  Hence, anything that makes the format less
> declarative adds to the complication and complicates debugging.  This is
> the same reason why I prefer a subsection for the new hotplug stuff---it
> makes the format more declarative and self-descriptive.
>
> Paolo
>
>>>> +        VMSTATE_PCI_HOTPLUG(acpi_pci_hotplug, PIIX4PMState,
>>>> +                            vmstate_test_use_acpi_pci_hotplug),
>>>
>>> This works, but it is a bit different from other cases that are already
>>> present, which use a subsection.  It is a bit ugly because it looks like
>>> a version-1 field, but in fact it is not version 1.
>>>
>>> I'll let other people decide whether it's acceptable or not, but I'm
>>> leaning towards asking you to use a subsection.
>>>
>>> Paolo