[v6,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 desirable 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>
---
 tests/qemu-iotests/206        |  4 ++--
 tests/qemu-iotests/iotests.py | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 5 deletions(-)

21.12.2018 12:35, 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 desirable 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>
> ---
>   tests/qemu-iotests/206        |  4 ++--
>   tests/qemu-iotests/iotests.py | 28 +++++++++++++++++++++++++---
>   2 files changed, 27 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..a96a7010d4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -246,10 +246,33 @@ 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.'''
> +    # Iterate through either lists or dicts;
> +    if isinstance(qmsg, list):
> +        items = enumerate(qmsg)
> +    else:
> +        items = qmsg.items()
> +
> +    for k, v in items:
> +        if isinstance(v, list) or isinstance(v, dict):
> +            qmsg[k] = filter_qmp(v, filter_fn)
> +        else:
> +            qmsg[k] = filter_fn(k, v)
> +    return qmsg

Hmm. This made me check, is enumerate applicable to dicts,
and, yes it is.

so, it may be as easy as

     for k, v in enumerate(qmsg):
         if isinstance(v, list) or isinstance(v, dict):
             qmsg[k] = filter_qmp(v, filter_fn)
         else:
             qmsg[k] = filter_fn(k, v)

with this:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


But does it what you want?
So, for lists of strings, filter_fn will get pair of index in array
and value?
On the other hand, we can adjust the behavior as we want when we introduce
such filters.

> +
>   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 +488,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/21/18 7:41 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hmm. This made me check, is enumerate applicable to dicts,
> and, yes it is.
> 

enumerate on dicts gives you a numerical index paired with the key,
so... k is the numerical index and v is the key.

> so, it may be as easy as
> 
>      for k, v in enumerate(qmsg):
>          if isinstance(v, list) or isinstance(v, dict):
>              qmsg[k] = filter_qmp(v, filter_fn)

                    ^ this will break if qmsg was a dict.

>          else:
>              qmsg[k] = filter_fn(k, v)
> 
> with this:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
> But does it what you want?
> So, for lists of strings, filter_fn will get pair of index in array
> and value?
> On the other hand, we can adjust the behavior as we want when we introduce
> such filters.

I'm not sure what I want...

Named index is the best, but isn't always available because like you
say, the object we are passed is not always a dictionary. We could
theoretically have a list of lists somewhere. What key would I pass
then? It's meaningless.

I could change qmp_filter to pass the k,v to the filter when it finds
the "last dictionary," i.e. if it recursively finds no more dictionaries
underneath, it passes the value along regardless of whatever it finds.
This is a bit more complex because I think I'd need a co-mutual
recursive function to "travel" each value and report if there are
further dictionaries down below.

Or, I could just leave the filter as-is where it only ever gets one,
non-dict/non-list item and receives a key. Sometimes the key will now be
a numerical index which is likely not useful...

I might stage this as-is for now. Let's revisit it before 4.0 rc0.

--js

21.12.2018 23:13, John Snow wrote:
> 
> 
> On 12/21/18 7:41 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hmm. This made me check, is enumerate applicable to dicts,
>> and, yes it is.
>>
> 
> enumerate on dicts gives you a numerical index paired with the key,
> so... k is the numerical index and v is the key.

hmm, yes, I was wrong. Ok, than I'm fine with your code. Take my r-b.

> 
>> so, it may be as easy as
>>
>>       for k, v in enumerate(qmsg):
>>           if isinstance(v, list) or isinstance(v, dict):
>>               qmsg[k] = filter_qmp(v, filter_fn)
> 
>                      ^ this will break if qmsg was a dict.
> 
>>           else:
>>               qmsg[k] = filter_fn(k, v)
>>
>> with this:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>
>> But does it what you want?
>> So, for lists of strings, filter_fn will get pair of index in array
>> and value?
>> On the other hand, we can adjust the behavior as we want when we introduce
>> such filters.
> 
> I'm not sure what I want...
> 
> Named index is the best, but isn't always available because like you
> say, the object we are passed is not always a dictionary. We could
> theoretically have a list of lists somewhere. What key would I pass
> then? It's meaningless.
> 
> I could change qmp_filter to pass the k,v to the filter when it finds
> the "last dictionary," i.e. if it recursively finds no more dictionaries
> underneath, it passes the value along regardless of whatever it finds.
> This is a bit more complex because I think I'd need a co-mutual
> recursive function to "travel" each value and report if there are
> further dictionaries down below.
> 
> Or, I could just leave the filter as-is where it only ever gets one,
> non-dict/non-list item and receives a key. Sometimes the key will now be
> a numerical index which is likely not useful...
> 
> I might stage this as-is for now. Let's revisit it before 4.0 rc0.
> 
> --js
>