|
From: | Pradeep Jagadeesh |
Subject: | Re: [Qemu-block] [PATCH v13 2/6] qmp: Use ThrottleLimits structure |
Date: | Mon, 13 Nov 2017 14:02:52 +0100 |
User-agent: | Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 10/13/2017 4:26 PM, 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 structure.{ 'struct': 'BlockIOThrottle', - 'data': { '*device': 'str', '*id': 'str', 'bps': 'int', 'bps_rd': 'int', - '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': 'int', - '*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 unexpected"}}" I just checked previous versions of the series and I see that Manos already warned you of this in v11: https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg04698.htmlOn 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 release. 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 '_').
Could you please have a look at Manos reply and my reply also. Please let me know what you think. -Pradeep
[Prev in Thread] | Current Thread | [Next in Thread] |