| Message ID | 20170720162815.19802-10-ldoktor@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <ldoktor@redhat.com> > --- > 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 > return self.cmd_obj(qmp_cmd) > > def command(self, cmd, **kwds): > -- > 2.9.4 >
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 <ldoktor@redhat.com> >> --- >> 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. Lukáš > >> return self.cmd_obj(qmp_cmd) >> >> def command(self, cmd, **kwds): >> -- >> 2.9.4 >> >
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 <ldoktor@redhat.com> > >> --- > >> 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)?
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 <ldoktor@redhat.com> >>>> --- >>>> 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áš
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 return self.cmd_obj(qmp_cmd) def command(self, cmd, **kwds):
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 <ldoktor@redhat.com> --- scripts/qmp/qmp.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)