On Fri, Dec 04, 2015 at 11:11:31AM +0100, Markus Armbruster wrote:
> David Gibson <da...@gibson.dropbear.id.au> writes:
>
> > On Fri, Dec 04, 2015 at 09:05:51AM +0100, Markus Armbruster wrote:
> >> David Gibson <da...@gibson.dropbear.id.au> 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)
> >> >> /p...@800000020000000.mmio[0] (qemu:memory-region)
> >> >> /p...@800000020000000.mmio-alias[0] (qemu:memory-region)
> >> >> /p...@800000020000000.io[0] (qemu:memory-region)
> >> >> /p...@800000020000000.io-alias[0] (qemu:memory-region)
> >> >> /pci.0 (PCI)
> >> >> /pci@800000020000000.iommu-root[0] (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.
I would like it to be null (or simply missing) in future.
> 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.
But leaving it as is for 2.5 is fine.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
