qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: John Snow <jsnow@redhat.com>,
	Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"qemu-block@nongnu.org" <qemu-block@nongnu.org>
Cc: Markus Armbruster <armbru@redhat.com>,
	Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function
Date: Wed, 19 Dec 2018 12:50:14 -0600	[thread overview]
Message-ID: <dd3979c0-2fd6-83a2-8b6f-7e3a2b828583@redhat.com> (raw)
In-Reply-To: <a1b2fca0-2bd1-01f6-6ff0-dfde78d4dfec@redhat.com>

On 12/19/18 11:55 AM, John Snow wrote:

>>> +def ordered_kwargs(kwargs):
>>> +    # kwargs prior to 3.6 are not ordered, so:
>>> +    od = OrderedDict()
>>> +    for k in sorted(kwargs.keys()):
>>
>> you can use for k, v in sorted(kwargs.items()):
>> and use then v instead of kwargs[k]
>>
> 
> I don't need to sort the tuples, though, Only the keys -- which are not
> duplicated. Is it really worth changing? ...

If I'm reading this correctly:
https://www.pythoncentral.io/how-to-sort-python-dictionaries-by-key-or-value/

sorting tuples with unique keys is the same as sorting by the keys, but 
gives you the value (as part of the tuple) for free.  So the benefit is 
that:

> 
>>> +        if isinstance(kwargs[k], dict):
>>> +            od[k] = ordered_kwargs(kwargs[k])

here, you'd write:

if isinstance(v, dict):
   od[k] = ordered_kwargs(v)

instead of having to repeat the value lookup.

> 
> The motivation is that log() will log whatever you give it and apply
> filters that work on that kind of object. Some callers need to filter
> rich QMP objects and some callers need to filter text -- this is the way
> log() behaves right now and I simply didn't change it.
> 
> What qmp_log currently does is convert both the outgoing and incoming
> QMP objects to text, and then filters them as text. However, only
> precisely one test (206) uses this functionality.
> 
> So... I need some way for test 206 to do what it does. One way is to
> make a rich QMP filter, which is what I do later in this series under
> the pretense that other tests will likely want to filter QMP output, too.
> 
> The other approach involves teaching qmp_log to accept two kinds of
> filters (qmp and text) and then passing both along to log(), which will
> then filter the object before pretty-printing and then apply the text
> filters after pretty-printing, and then logging the result.
> 
> As it stands now, though, log() just applies all filters to the first
> argument without the caller needing to disambiguate. If I teach log() to
> use two types of filters, I need to go back through all of the iotests
> and teach all callers to specify e.g. qfilters= or tfilters=.
> 
> I opted for an approach that let me just edit test 206 instead -- and
> one that added a recursive QMP filter that might be useful in the future
> for other purposes.

The reasoning here makes sense to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

  reply	other threads:[~2018-12-19 18:50 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-19  1:52 [Qemu-devel] [PATCH v4 0/5] bitmaps: remove x- prefix from QMP api Part2 John Snow
2018-12-19  1:52 ` [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function John Snow
2018-12-19 10:20   ` Vladimir Sementsov-Ogievskiy
2018-12-19 17:55     ` John Snow
2018-12-19 18:50       ` Eric Blake [this message]
2018-12-19 18:52   ` Eric Blake
2018-12-19 18:57     ` John Snow
2018-12-19 19:19       ` Eric Blake
2018-12-19 19:47         ` John Snow
2018-12-19  1:52 ` [Qemu-devel] [PATCH v4 2/5] iotests: remove default filters from qmp_log John Snow
2018-12-19 10:58   ` Vladimir Sementsov-Ogievskiy
2018-12-19  1:52 ` [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only John Snow
2018-12-19 11:07   ` Vladimir Sementsov-Ogievskiy
2018-12-19 11:27     ` Vladimir Sementsov-Ogievskiy
2018-12-19 17:29       ` John Snow
2018-12-19 19:01       ` Eric Blake
2018-12-19 19:52         ` John Snow
2018-12-20  9:33           ` Vladimir Sementsov-Ogievskiy
2018-12-19 18:35     ` John Snow
2018-12-20  9:11       ` Vladimir Sementsov-Ogievskiy
2018-12-19  1:52 ` [Qemu-devel] [PATCH v4 4/5] iotests: implement pretty-print for log and qmp_log John Snow
2018-12-19  1:52 ` [Qemu-devel] [PATCH v4 5/5] iotests: add iotest 236 for testing bitmap merge John Snow
2018-12-19 19:34   ` Eric Blake
2018-12-20  2:01     ` John Snow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dd3979c0-2fd6-83a2-8b6f-7e3a2b828583@redhat.com \
    --to=eblake@redhat.com \
    --cc=armbru@redhat.com \
    --cc=jsnow@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=mreitz@redhat.com \
    --cc=qemu-block@nongnu.org \
    --cc=qemu-devel@nongnu.org \
    --cc=vsementsov@virtuozzo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).