Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Vladimir Sementsov-Ogievskiy]

19.12.2018 21:35, John Snow wrote:
> 
> 
> On 12/19/18 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2018 4:52, John Snow wrote:
>>> log() treats filters as if they can always filter its primary argument.
>>> qmp_log treats filters as if they're always text.
>>>
>>> Change qmp_log to treat filters as if they're always qmp object filters,
>>> then change the logging call to rely on log()'s ability to serialize QMP
>>> objects, so we're not duplicating that effort.
>>
>> As I understand, there still no use for qmp-object based filters (even after 
>> the
>> series), do we really need them? I'm afraid it's premature complication.
>>
> 
> There are callers of log() that use qmp filters. Those callers can now
> migrate over to qmp_log to get both the call and response.
> 
> There ARE users of QMP filters.
> 
> Look at `git grep 'log(vm'` for callers that are passing rich QMP
> objects. The ones that pass filters are usually passing filter_qmp_event.

oops, I'm totally ashamed)

Interesting, about least surprising behavior, for me it's a surprise. I thought,
that assumed behavior is filtering final text..

> 
> Now, if we choose, we can move them over to using qmp_log and amend the
> logging output to get both the outgoing and returning message.
> 
> -- hmm, maybe, if you want, and I am NOT suggesting I will do this
> before the holiday break (and therefore not in this series) -- what we
> can do is this:
> 
> log(txt, filters=[]) -- Takes text and text filters only.
> 
> log_qmp(obj, tfilters=[], qfilters=[]) -- Logs a QMP object, takes QMP
> filters for pre-filtering and tfilters for post-filtering. Contains the
> json.dumps call. Simply passes tfilters down to log().
> 
> vm.qmp(log=1, tfilters=[], qfilters=[], ...) -- Perform the actual QMP
> call and response, logging the outgoing and incoming objects via log_qmp.
> 
> I can use this patchset as a starting point to do that. It will involve
> amending a lot of existing tests and test outputs, so I won't do this
> unless there appears to be some support for that API in advance.

Ohm, stop listening me, I had false assumptions. Ok, at this point, I'll move to
reviewing your next series, hope it didn't complicated due to my false 
criticism.

> 
>> Why not to keep all filters text based? If we need to filter some concrete 
>> fields,
>> not the whole thing, I doubt that recursion and defining functions inside
>> functions is a true way for it. Instead in concrete case, like in you test, 
>> it's
>> better to select fields that should be in output, and output only them.
>>
>>>
>>> Because kwargs have been sorted already, the order is preserved.
>>>
>>> Edit the only caller who uses filters on qmp_log to use a qmp version,
>>> also added in this patch.
>>> ---
>>>    tests/qemu-iotests/206        |  4 ++--
>>>    tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>>>    2 files changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
>>> index e92550fa59..5bb738bf23 100755
>>> --- a/tests/qemu-iotests/206
>>> +++ b/tests/qemu-iotests/206
>>> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>>>    
>>>    def blockdev_create(vm, options):
>>>        result = vm.qmp_log('blockdev-create',
>>> -                        filters=[iotests.filter_testfiles],
>>> +                        filters=[iotests.filter_qmp_testfiles],
>>>                            job_id='job0', options=options)
>>>    
>>>        if 'return' in result:
>>> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>>>                              'size': 0 })
>>>    
>>>        vm.qmp_log('blockdev-add',
>>> -               filters=[iotests.filter_testfiles],
>>> +               filters=[iotests.filter_qmp_testfiles],
>>>                   driver='file', filename=disk_path,
>>>                   node_name='imgfile')
>>>    
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 55fb60e039..812302538d 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>>>            event['timestamp']['microseconds'] = 'USECS'
>>>        return event
>>>    
>>> +def filter_qmp(qmsg, filter_fn):
>>> +    '''Given a string filter, filter a QMP object's values.
>>> +    filter_fn takes a (key, value) pair.'''
>>> +    for key in qmsg:
>>> +        if isinstance(qmsg[key], list):
>>> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
>>> +        elif isinstance(qmsg[key], dict):
>>> +            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
>>> +        else:
>>> +            qmsg[key] = filter_fn(key, qmsg[key])
>>> +    return qmsg
>>> +
>>>    def filter_testfiles(msg):
>>>        prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>>        return msg.replace(prefix, 'TEST_DIR/PID-')
>>>    
>>> +def filter_qmp_testfiles(qmsg):
>>> +    def _filter(key, value):
>>> +        if key == 'filename' or key == 'backing-file':
>>> +            return filter_testfiles(value)
>>> +        return value
>>> +    return filter_qmp(qmsg, _filter)
>>> +
>>>    def filter_generated_node_ids(msg):
>>>        return re.sub("#block[0-9]+", "NODE_NAME", msg)
>>>    
>>> @@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
>>>        def qmp_log(self, cmd, filters=[], **kwargs):
>>>            full_cmd = OrderedDict({"execute": cmd,
>>>                                    "arguments": ordered_kwargs(kwargs)})
>>> -        logmsg = json.dumps(full_cmd)
>>> -        log(logmsg, filters)
>>> +        log(full_cmd, filters)
>>>            result = self.qmp(cmd, **kwargs)
>>> -        log(json.dumps(result, sort_keys=True), filters)
>>> +        log(result, filters)
>>>            return result
>>>    
>>>        def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>
>>
>>
> 


-- 
Best regards,
Vladimir