[v5,09/11] iotests: change qmp_log filters to expect QMP objects only

As laid out in the previous commit's message:

```
Several places in iotests deal with serializing objects into JSON
strings, but to add pretty-printing it seems desireable to localize
all of those cases.

log() seems like a good candidate for that centralized behavior.
log() can already serialize json objects, but when it does so,
it assumes filters=[] operates on QMP objects, not strings.

qmp_log currently operates by dumping outgoing and incoming QMP
objects into strings and filtering them assuming that filters=[]
are string filters.
```

Therefore:

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.

Add a qmp version of filter_testfiles and adjust the only caller using
it for qmp_log to use the qmp version.

Signed-off-by: John Snow  <jsnow@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/206        |  4 ++--
 tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)

On 12/19/18 8:29 PM, John Snow wrote:
> As laid out in the previous commit's message:
> 
> ```
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize

s/desireable/desirable/

> all of those cases.
> 
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
> 
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
> ```
> 
> Therefore:
> 
> 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.
> 
> Add a qmp version of filter_testfiles and adjust the only caller using
> it for qmp_log to use the qmp version.
> 
> Signed-off-by: John Snow  <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>

Odd double S-o-B differing only by space.

> ---
>   tests/qemu-iotests/206        |  4 ++--
>   tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>   2 files changed, 23 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

20.12.2018 5:29, John Snow wrote:
> As laid out in the previous commit's message:
> 
> ```
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize
> all of those cases.
> 
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
> 
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
> ```
> 
> Therefore:
> 
> 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.
> 
> Add a qmp version of filter_testfiles and adjust the only caller using
> it for qmp_log to use the qmp version.
> 
> Signed-off-by: John Snow  <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   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 57fe20db45..dcd0c6f71d 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.'''

hm, I decided to look into PEP8, which in turn refers to PEP257,
which asks:
  - For consistency, always use """triple double quotes""" around docstring
  - Unless the entire docstring fits on a line, place the closing quotes on a line by themselves

Unfortunately, iotests.py prefers to be in opposition.. And consistency within the file is more
important. May be we'll fix it one day..


> +    for key in qmsg:

and here again we can benefit (or all right-value qmsg[key]) of using qmsg.items()

> +        if isinstance(qmsg[key], list):
> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]

hmm, stop. filter_qmp() assumes that its argument is dict. but atom may not be dict.

so, to fit into the concept of fn(key, value) filtering function, we should do something like
this:

for i in len(qmsg[key]):
   if isinstance(qmsg[key], dict):
     qmsg[key][i] = filter_qmp(qmsg[key][i], filter_fn)

qmsg[key] = filter_fn(key, qmsg[key])

---
or, we may want to apply filter_fn only to lists of non-dicts, and filter only list of dicts,
assuming that we don't have mixed lists.


> +        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)
>   
> @@ -465,10 +484,9 @@ class VM(qtest.QEMUQtestMachine):
>               ("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):
>

On 12/19/18 9:53 PM, Eric Blake wrote:
> On 12/19/18 8:29 PM, John Snow wrote:
>> As laid out in the previous commit's message:
>>
>> ```
>> Several places in iotests deal with serializing objects into JSON
>> strings, but to add pretty-printing it seems desireable to localize
> 
> s/desireable/desirable/
> 
>> all of those cases.
>>
>> log() seems like a good candidate for that centralized behavior.
>> log() can already serialize json objects, but when it does so,
>> it assumes filters=[] operates on QMP objects, not strings.
>>
>> qmp_log currently operates by dumping outgoing and incoming QMP
>> objects into strings and filtering them assuming that filters=[]
>> are string filters.
>> ```
>>
>> Therefore:
>>
>> 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.
>>
>> Add a qmp version of filter_testfiles and adjust the only caller using
>> it for qmp_log to use the qmp version.
>>
>> Signed-off-by: John Snow  <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Odd double S-o-B differing only by space.

I fixed my auto-signer! It has rudely detected my typo and decided that
it needed a fresh SOB.

> 
>> ---
>>   tests/qemu-iotests/206        |  4 ++--
>>   tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
Thanks!

On 12/20/18 6:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2018 5:29, John Snow wrote:
>> As laid out in the previous commit's message:
>>
>> ```
>> Several places in iotests deal with serializing objects into JSON
>> strings, but to add pretty-printing it seems desireable to localize
>> all of those cases.
>>
>> log() seems like a good candidate for that centralized behavior.
>> log() can already serialize json objects, but when it does so,
>> it assumes filters=[] operates on QMP objects, not strings.
>>
>> qmp_log currently operates by dumping outgoing and incoming QMP
>> objects into strings and filtering them assuming that filters=[]
>> are string filters.
>> ```
>>
>> Therefore:
>>
>> 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.
>>
>> Add a qmp version of filter_testfiles and adjust the only caller using
>> it for qmp_log to use the qmp version.
>>
>> Signed-off-by: John Snow  <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   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 57fe20db45..dcd0c6f71d 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.'''
> 
> hm, I decided to look into PEP8, which in turn refers to PEP257,
> which asks:
>   - For consistency, always use """triple double quotes""" around docstring
>   - Unless the entire docstring fits on a line, place the closing quotes on a line by themselves
> 
> Unfortunately, iotests.py prefers to be in opposition.. And consistency within the file is more
> important. May be we'll fix it one day..
> 
> 
>> +    for key in qmsg:
> 
> and here again we can benefit (or all right-value qmsg[key]) of using qmsg.items()
> 
>> +        if isinstance(qmsg[key], list):
>> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
> 
> hmm, stop. filter_qmp() assumes that its argument is dict. but atom may not be dict.
> 

Oh, good catch. Will fix.

> so, to fit into the concept of fn(key, value) filtering function, we should do something like
> this:
> 
> for i in len(qmsg[key]):
>    if isinstance(qmsg[key], dict):
>      qmsg[key][i] = filter_qmp(qmsg[key][i], filter_fn)
> 
> qmsg[key] = filter_fn(key, qmsg[key])
> 
> ---
> or, we may want to apply filter_fn only to lists of non-dicts, and filter only list of dicts,
> assuming that we don't have mixed lists.
> 
> 
>> +        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)
>>   
>> @@ -465,10 +484,9 @@ class VM(qtest.QEMUQtestMachine):
>>               ("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):
>>
> 
>