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

[Eduardo Habkost]

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)?

-- 
Eduardo