[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v13 2/6] qmp: Use ThrottleLimits structure

From: Pradeep Jagadeesh
Subject: Re: [Qemu-devel] [PATCH v13 2/6] qmp: Use ThrottleLimits structure
Date: Mon, 6 Nov 2017 13:52:03 +0100
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0

On 11/6/2017 10:35 AM, Manos Pitsidianakis wrote:
On Fri, Oct 13, 2017 at 09:26:17AM -0500, Eric Blake wrote:
[adding Markus, and block list]

On 10/13/2017 09:16 AM, Alberto Garcia wrote:
On Mon 02 Oct 2017 04:33:28 PM CEST, Pradeep Jagadeesh wrote:
This patch factors out code to use the ThrottleLimits

 { 'struct': 'BlockIOThrottle',
-  'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd':
-            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
'iops_wr': 'int',
-            '*bps_max': 'int', '*bps_rd_max': 'int',
-            '*bps_wr_max': 'int', '*iops_max': 'int',
-            '*iops_rd_max': 'int', '*iops_wr_max': 'int',
-            '*bps_max_length': 'int', '*bps_rd_max_length': 'int',
-            '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
-            '*iops_rd_max_length': 'int', '*iops_wr_max_length':
-            '*iops_size': 'int', '*group': 'str' } }
+  'base': 'ThrottleLimits',
+  'data': { '*device': 'str', '*id': 'str', '*group': 'str' } }

So BlockIOThrottle used to have parameters named bps_rd and iops_wr, and
after this patch they become bps-read and iops-write. This breaks the
API completely, as you can see if you run e.g. iotest 129:

AssertionError: failed path traversal for "return" in "{u'error':
{u'class': u'GenericError', u'desc': u"Parameter 'iops_rd' is

I just checked previous versions of the series and I see that Manos
already warned you of this in v11:


On the bright side, ThrottleLimits is marked 'since 2.11' (added in
commit 432d889e), meaning it has not yet been released, so we CAN fix
the naming in ThrottleLimits to be compatible with BlockIOThrottle if we
want to share the type, as long as we get it done before the 2.11

We decided to keep BlockIOThrottle separate from ThrottleLimits because
that would break the old I/O throttling API, just like is done in this
patch series.  BlockIOThrottle is the one using old naming conventions
so I think it should be the one to go, if that has to be done.

But this all boils down to whether the legacy throttling API has to
break in 2.11 or not, which probably is the maintainer's decision.

It does mean tweaking Manos' code to use compatible names
everywhere, but that may be a wise course of action (we tend to favor
'-' in new API names unless there is a strong reason to use '_'; but
sharing code for maximum back-compat would be a reason to use '_').

Thanks for your reply Manos.

@Eric, So shall I go ahead and revert my patches to as before. I mean using iothrottle structure?.
What is your suggestion?

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]