Re: [Qemu-devel] Posible regressions around spapr-dr-connector property drc-connector[]

[Markus Armbruster]

David Gibson <address@hidden> writes:

> On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote:
>> David Gibson <address@hidden> writes:
>> 
>> > On Thu, Dec 03, 2015 at 04:30:31PM +0100, Markus Armbruster wrote:
>> >> 1. Before commit 94649d4 "spapr: Don't use QOM [*] syntax for DR
>> >>    connectors", the indexes were small integers:
>> >> 
>> >>        (qemu) info qom-tree
>> >>        /machine (pseries-2.4-machine)
>> >>          /unattached (container)
>> >>            [...]
>> >>            /device[5] (spapr-pci-host-bridge)
>> >>              /address@hidden (qemu:memory-region)
>> >>              /address@hidden (qemu:memory-region)
>> >>              /address@hidden (qemu:memory-region)
>> >>              /address@hidden (qemu:memory-region)
>> >>              /pci.0 (PCI)
>> >>              /address@hidden (qemu:memory-region)
>> >>              /dr-connector[0] (spapr-dr-connector)
>> >>              /dr-connector[1] (spapr-dr-connector)
>> >>              /dr-connector[2] (spapr-dr-connector)
>> >>              [...]
>> >> 
>> >>    Since then, they're big ones:
>> >> 
>> >>              /dr-connector[1073741824] (spapr-dr-connector)
>> >>              /dr-connector[1073741825] (spapr-dr-connector)
>> >>              /dr-connector[1073741826] (spapr-dr-connector)
>> >> 
>> >>    The commit message doesn't quite spell out this change, and I'm
>> >>    therefore double-checkint it's intentional.  Is it?
>> >
>> > Yes, it's intentional.  The small integers were arbitrarily allocated
>> > by the QOM magic [*] code, whereas the big integers are actually
>> > meaningful values (essentially the DRC's global ID for the dynamic
>> > reconfiguration hypervisor interfaces).
>> 
>> Good.
>> 
>> >> 2. Before commit 6c2f9a1 "qapi: Make output visitor return qnull()
>> >>    instead of NULL", qom-get returned {}:
>> >> 
>> >>    Since then, it returns null:
>> >> 
>> >>        QMP> { "execute": "qom-get", "arguments": { "path": 
>> >> "/machine/unattached/device[5]/dr-connector[1073741950]", "property": 
>> >> "fdt" } }
>> >>        {"return": null}
>> >> 
>> >>    Does anyone care?
>> >
>> > Hm, I'm guessing this is a case where fdt is NULL internally.  Which I
>> 
>> Yes.
>> 
>> > think will happen before a device gets hotplugged into the DRC.  In
>> > that case null seems more correct to me than {}, since {} would also
>> > be what's shown for a present-but-empty device tree.
>> 
>> It was {} in 2.4.  Changing it to null so we can distingish "nothing"
>> from "empty" is an incompatible change.  May make sense anyway, but I
>> can't judge it.
>
> Strictly speaking it's an incompatible change, yes.  But I find it
> hard to imagine anything would be relying on the {} behaviour.  This
> property is essentially a debugging interface to start with, and the
> missing / empty case is examining it in a state that's unlikely to be
> interesting.

I'm not against changing it, I just want it changed intentionally rather
than by accidental side effect :)

If you tell me you want null here going forward, I'll make sure it gets
changed to null in the next development cycle, with a nice commit
message.

If you want it to be null in 2.5, NAK "[PATCH for-2.5 2/3] spapr_drc:
Change value of property "fdt" from null back to {}".  It'll remain an
implicit change then, not documented in commit messages.  I expect we'll
eventually get a patch similar to the NAKed one regardless, because
we'll tighten up the visitor contracts, and returning without visiting
anything should become a programming error then.