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

[John Snow]

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.

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.

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.

> Here's my review from a non-Python export viewpoint:
> 
>>
>> We can accomplish this by sorting **kwargs into an OrderedDict,
>> which does preserve addition order.
>> ---
>>   tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
> 
>> @@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
>>           return result
>>         def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>> +        full_cmd = OrderedDict({"execute": cmd,
>> +                                "arguments": ordered_kwargs(kwargs)})
>> +        logmsg = json.dumps(full_cmd)
> 
> Vladimir knows Python better than me, but once you fix this OrderedDict
> construction up to actually work, the patch looks reasonable to me.
> 

Yeah, it only worked by accident of implementation details :(
I've patched it up locally.

--js