Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[John Snow]

On 12/19/18 2:19 PM, Eric Blake wrote:
> On 12/19/18 12:57 PM, John Snow wrote:
>>
>>
>> On 12/19/18 1:52 PM, Eric Blake wrote:
>>> On 12/18/18 7:52 PM, John Snow wrote:
>>>> Python before 3.6 does not sort kwargs by default.
>>>> If we want to print out pretty-printed QMP objects while
>>>> preserving the "exec" > "arguments" ordering, we need a custom sort.
>>>
>>> Naive question - why do we need the sorting? Is it so that the output is
>>> deterministic?  Surely it can't be because the ordering otherwise makes
>>> a difference to execution.
>>>
>>
>> Yeah, it's because dicts are unordered and prior to 3.6 they may shuffle
>> around arbitrarily based on internal hashes.
>>
>> kwargs in particular are unordered- the order we send over the wire may
>> or may not reflect the order the test author wrote in their iotest.
> 
> Which shouldn't matter to what QMP executes, but MIGHT matter in getting
> reproducible log output.
> 
>>
>> Therefore, it's a way to get consistent ordering.
>>
>> Now, we CAN just rely on sort_keys=True to be done with it, however,
>> this puts arguments before execute, and it's less nice to read -- and
>> I'd have to change a LOT of test output.
> 
> Okay, I'm finally seeing it; the existing code has:
> 
>     def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>         logmsg = '{"execute": "%s", "arguments": %s}' % \
>             (cmd, json.dumps(kwargs, sort_keys=True))
> 
> where our log is outputting a message that resembles a dict where
> "execute": is the first key, and the user's input dict is then sorted
> (the top-most output of {} is unsorted, but the nested ones are sorted,
> and possibly in a different order than the user typed them, but at least
> deterministic). But when you change to the user passing a full QMP
> command (rather than just a dict for the arguments of the QMP command),
> using sort_keys=True will sort everything _including_ putting
> "arguments" before "execute" (which is deterministic but changes log
> output); while using sort_keys=False will not affect output in newer
> python where kwargs remains ordered, but risks nondeterministic output
> on older python.
> 

Yes, and pretty-printing requires the full object, otherwise it's too
hard to manually re-indent the buffered subdict, and you have to indent
the outer dict, and...

...

It's easier to just build the full dictionary and then pretty-print it.

The motivation here is that pretty-printing a call to qmp_log should
indent both outgoing and incoming because that's semantically the most
obvious and right thing to do. Principle of least surprise.

>>
>> This way keeps the order you expect to see while maintaining a strict
>> order for the arguments. I think that's the nicest compromise until we
>> can stipulate 3.6+ in the year 2034 where kwargs *are* strictly ordered.
> 
> But kwargs strict ordering is only at the top-level, not recursive,
> right? That is, even if you can rely on:
> 
> foo(b=1, a=2)
> 
> providing kwargs where 'b' always outputs before 'a', I don't see how
> that would help:
> 
> foo(b={'d':3, 'c':4})
> 
> not reorder the keys within 'b' without your recursive ordering.
> 

Well, kwargs *are* dictionaries, and both become sorted in 3.6. You can
rely on the ordering at any level, but only in 3.6 and after.

We cannot rely on that behavior, though, so in our current reality:

(1) kwargs can be arbitrarily reordered
(2) subdictionaries in kwargs can also be arbitrarily reordered

And so my ordered_kwargs() function recursively orders any dictionaries
it finds, assuming a typical JSON structure -- which will suffice for
QMP objects.

In practice, it appears to correctly re-implement the behavior of
json.dumps(..., sort_keys=True).

--js