Re: [Qemu-devel] [PATCH 09/11] qmp.py: Avoid overriding a builtin object

[Lukáš Doktor]

Dne 21.7.2017 v 20:46 Eduardo Habkost napsal(a):
> On Fri, Jul 21, 2017 at 08:53:49AM +0200, Lukáš Doktor wrote:
>> Dne 20.7.2017 v 20:38 Eduardo Habkost napsal(a):
>>> On Thu, Jul 20, 2017 at 06:28:13PM +0200, Lukáš Doktor wrote:
>>>> The "id" is a builtin method to get object's identity and should not be
>>>> overridden. This might bring some issues in case someone was directly
>>>> calling "cmd(..., id=id)" but I haven't found such usage on brief search
>>>> for "cmd\(.*id=".
>>>>
>>>> Signed-off-by: Lukáš Doktor <address@hidden>
>>>> ---
>>>>  scripts/qmp/qmp.py | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
>>>> index a14b001..c3e0206 100644
>>>> --- a/scripts/qmp/qmp.py
>>>> +++ b/scripts/qmp/qmp.py
>>>> @@ -177,19 +177,19 @@ class QEMUMonitorProtocol(object):
>>>>              print >>sys.stderr, "QMP:<<< %s" % resp
>>>>          return resp
>>>>  
>>>> -    def cmd(self, name, args=None, id=None):
>>>> +    def cmd(self, name, args=None, cmd_id=None):
>>>>          """
>>>>          Build a QMP command and send it to the QMP Monitor.
>>>>  
>>>>          @param name: command name (string)
>>>>          @param args: command arguments (dict)
>>>> -        @param id: command id (dict, list, string or int)
>>>> +        @param cmd_id: command id (dict, list, string or int)
>>>>          """
>>>>          qmp_cmd = {'execute': name}
>>>>          if args:
>>>>              qmp_cmd['arguments'] = args
>>>> -        if id:
>>>> -            qmp_cmd['id'] = id
>>>> +        if cmd_id:
>>>> +            qmp_cmd['cmd_id'] = cmd_id
>>>
>>> The member sent through the monitor should still be called "id".
>>> i.e.:
>>>
>>>     qmp_cmd['id'] = cmd_id
>>>
>> Oups, sorry, automatic rename changed it and I forgot to fix
>> this one back. I'll address this in v2. The main problem with
>> this patch is it could break named arguments (`cmd(..., id=id)`
>> calls) so I'm not sure it's worth including. But as mentioned
>> in commit message I grepped full sources so we better fix this
>> before someone starts using it.
> 
> I'm not convinced we need this patch, either.  What exactly
> breaks if we don't apply this patch and somebody tries to use
> cmd(..., id=id)?
> 

It'll work properly. The only issue is the `id` method will be unavailable 
inside that function. So let's say you want to add `print` method to debug 
issue with duplicate arguments, you'd be unable to use `id` to distinguish 
between them. The other issue is that people will see it and copy&paste this 
ugliness to other projects poisoning the world :-)

Therefor I'd like to include it, but I'm prepared to yield.

Lukáš

signature.asc
Description: OpenPGP digital signature