| Message ID | 1499368127-19855-1-git-send-email-chugh.ishani@research.iiit.ac.in |
|---|---|
| State | New |
| Headers | show |
On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote: Commit messages usually begin with a prefix indicating the affected component. This makes it easier for code reviewers and maintainers to browse email subjects and the git log. I recommend the following commit message: scripts/qmp: Python3 Support for qmp.py > def __json_read(self, only_event=False): > while True: > - data = self.__sockfile.readline() > + data = self.__sock_readline() > if not data: > return > resp = json.loads(data) > if 'event' in resp: > if self._debug: > - print >>sys.stderr, "QMP:<<< %s" % resp > + print("QMP:<<< %s" % resp) Previously debug output was printed to stderr. Please do not change this. Here is how to keep stderr in Python 2.6+/3 compatible fashion: from __future__ import print_function ... print("QMP:<<< %s" % resp, file=sys.stderr) There are more instances of this below. > @@ -98,9 +112,11 @@ class QEMUMonitorProtocol: > try: > self.__json_read() > except socket.error as err: > - if err[0] == errno.EAGAIN: > + if err.errno == errno.EAGAIN: > # No data available > pass > + else: > + raise > self.__sock.setblocking(1) > > # Wait for new events, if needed. The "else: raise" silently fixes a bug. Please mention that socket errors were silently ignored in the commit description. That will make the intention of this change clear for anyone who looks at this commit in the future. > @@ -175,7 +190,7 @@ class QEMUMonitorProtocol: > @param args: command arguments (dict) > @param id: command id (dict, list, string or int) > """ > - qmp_cmd = { 'execute': name } > + qmp_cmd = {'execute': name} > if args: > qmp_cmd['arguments'] = args > if id: Please make PEP8 fixes in a separate commit and only Python 2/3 compatibility changes in this commit. Commits that do only one thing are easier to understand, backport, and bisect. > @@ -245,3 +260,4 @@ class QEMUMonitorProtocol: > > def is_scm_available(self): > return self.__sock.family == socket.AF_UNIX > + What is the purpose of this extra newline?
On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote: > This patch intends to make qmp.py compatible with both python2 and python3. > > * Python 3 does not have dict.has_key(key), use key in dict instead > * Avoid line-based I/O since Python 2/3 have different character > encoding behavior. Explicitly encode/decode JSON UTF-8. > * Replace print by print function. If you're going todo this, then you need to actually import the python3 compatible print function - not just call the python2 print statement with brackets, as the semantics are different: $ python2 >>> print("foo", "bar") ('foo', 'bar') >>> from __future__ import print_function >>> print("foo", "bar") foo bar Only the latter matches python3 semantics > > Signed-off-by: Ishani Chugh <chugh.ishani@research.iiit.ac.in> > --- > scripts/qmp/qmp.py | 58 ++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 21 deletions(-) > > diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py > index 62d3651..58fb7d1 100644 > --- a/scripts/qmp/qmp.py > +++ b/scripts/qmp/qmp.py > @@ -42,6 +42,7 @@ class QEMUMonitorProtocol: > self.__address = address > self._debug = debug > self.__sock = self.__get_sock() > + self.__data = b"" > if server: > self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) > self.__sock.bind(self.__address) > @@ -56,7 +57,7 @@ class QEMUMonitorProtocol: > > def __negotiate_capabilities(self): > greeting = self.__json_read() > - if greeting is None or not greeting.has_key('QMP'): > + if greeting is None or 'QMP' not in greeting: > raise QMPConnectError > # Greeting seems ok, negotiate capabilities > resp = self.cmd('qmp_capabilities') > @@ -64,15 +65,28 @@ class QEMUMonitorProtocol: > return greeting > raise QMPCapabilitiesError > > + def __sock_readline(self): > + while True: > + ch = self.__sock.recv(1) > + if ch is None: > + if self.__data: > + raise ValueError('socket closed mid-line') > + return None > + self.__data += ch > + if ch == b'\n': > + line = self.__data.decode('utf-8') > + self.__data = b"" > + return line > + > def __json_read(self, only_event=False): > while True: > - data = self.__sockfile.readline() > + data = self.__sock_readline() So originally we could get back a "str" on python2, but now we get a "str" (which is unicode) on python2, but "unicode" on python2. > if not data: > return > resp = json.loads(data) This means the 'resp' now contains "unicode" data too on python2 instead of 'str'. This hopefully isn't a problem, but we certainly need to make sure the iotests still pass on py2, as it could well have a ripple effect in this respect. Have you run the iotests with this change applied ? > if 'event' in resp: > if self._debug: > - print >>sys.stderr, "QMP:<<< %s" % resp > + print("QMP:<<< %s" % resp) This is changing from printing to stderr, to printing to stdout which is not desirable. Likewise all the similar changes below. > self.__events.append(resp) > if not only_event: > continue > @@ -87,10 +101,10 @@ class QEMUMonitorProtocol: > @param wait (bool): block until an event is available. > @param wait (float): If wait is a float, treat it as a timeout value. > > - @raise QMPTimeoutError: If a timeout float is provided and the timeout > - period elapses. > - @raise QMPConnectError: If wait is True but no events could be retrieved > - or if some other error occurred. > + @raise QMPTimeoutError: If a timeout float is provided and the > + timeout period elapses. > + @raise QMPConnectError: If wait is True but no events could be > + retrieved or if some other error occurred. > """ Don't mix whitespace changes in with other functional changes. > > # Check for new events regardless and pull them into the cache: > @@ -98,9 +112,11 @@ class QEMUMonitorProtocol: > try: > self.__json_read() > except socket.error as err: > - if err[0] == errno.EAGAIN: > + if err.errno == errno.EAGAIN: > # No data available > pass > + else: > + raise > self.__sock.setblocking(1) > > # Wait for new events, if needed. > @@ -128,7 +144,6 @@ class QEMUMonitorProtocol: > @raise QMPCapabilitiesError if fails to negotiate capabilities > """ > self.__sock.connect(self.__address) > - self.__sockfile = self.__sock.makefile() > if negotiate: > return self.__negotiate_capabilities() > > @@ -143,7 +158,6 @@ class QEMUMonitorProtocol: > """ > self.__sock.settimeout(15) > self.__sock, _ = self.__sock.accept() > - self.__sockfile = self.__sock.makefile() > return self.__negotiate_capabilities() > > def cmd_obj(self, qmp_cmd): > @@ -155,16 +169,17 @@ class QEMUMonitorProtocol: > been closed > """ > if self._debug: > - print >>sys.stderr, "QMP:>>> %s" % qmp_cmd > + print("QMP:>>> %s" % qmp_cmd) > try: > - self.__sock.sendall(json.dumps(qmp_cmd)) > + command = json.dumps(qmp_cmd) > + self.__sock.sendall(command.encode('UTF-8')) > except socket.error as err: > - if err[0] == errno.EPIPE: > + if err.errno == errno.EPIPE: > return > - raise socket.error(err) > + raise > resp = self.__json_read() > if self._debug: > - print >>sys.stderr, "QMP:<<< %s" % resp > + print("QMP:<<< %s" % resp) > return resp > > def cmd(self, name, args=None, id=None): > @@ -175,7 +190,7 @@ class QEMUMonitorProtocol: > @param args: command arguments (dict) > @param id: command id (dict, list, string or int) > """ > - qmp_cmd = { 'execute': name } > + qmp_cmd = {'execute': name} Again bogus whitespace changes > if args: > qmp_cmd['arguments'] = args > if id: > @@ -184,7 +199,7 @@ class QEMUMonitorProtocol: > > def command(self, cmd, **kwds): > ret = self.cmd(cmd, kwds) > - if ret.has_key('error'): > + if 'error' in ret: > raise Exception(ret['error']['desc']) > return ret['return'] > > @@ -197,8 +212,8 @@ class QEMUMonitorProtocol: > > @raise QMPTimeoutError: If a timeout float is provided and the timeout > period elapses. > - @raise QMPConnectError: If wait is True but no events could be retrieved > - or if some other error occurred. > + @raise QMPConnectError: If wait is True but no events could be > + retrieved or if some other error occurred. And again > > @return The first available QMP event, or None. > """ > @@ -217,8 +232,8 @@ class QEMUMonitorProtocol: > > @raise QMPTimeoutError: If a timeout float is provided and the timeout > period elapses. > - @raise QMPConnectError: If wait is True but no events could be retrieved > - or if some other error occurred. > + @raise QMPConnectError: If wait is True but no events could be > + retrieved or if some other error occurred. Same > > @return The list of available QMP events. > """ > @@ -245,3 +260,4 @@ class QEMUMonitorProtocol: > > def is_scm_available(self): > return self.__sock.family == socket.AF_UNIX > + Adding trailing blank line to the file is bad. Regards, Daniel
Ishani Chugh <chugh.ishani@research.iiit.ac.in> writes: > This patch intends to make qmp.py compatible with both python2 and python3. > > * Python 3 does not have dict.has_key(key), use key in dict instead > * Avoid line-based I/O since Python 2/3 have different character > encoding behavior. Explicitly encode/decode JSON UTF-8. > * Replace print by print function. > > Signed-off-by: Ishani Chugh <chugh.ishani@research.iiit.ac.in> $ scripts/get_maintainer.pl -f $your_patch Markus Armbruster <armbru@redhat.com> (supporter:QMP) qemu-devel@nongnu.org (open list:All patches CC here) Just sayin' :)
----- On Jul 7, 2017, at 4:31 PM, Daniel P. Berrange berrange@redhat.com wrote: > On Fri, Jul 07, 2017 at 12:38:47AM +0530, Ishani Chugh wrote: >> This patch intends to make qmp.py compatible with both python2 and python3. >> >> * Python 3 does not have dict.has_key(key), use key in dict instead >> * Avoid line-based I/O since Python 2/3 have different character >> encoding behavior. Explicitly encode/decode JSON UTF-8. >> * Replace print by print function. > > If you're going todo this, then you need to actually import the python3 > compatible print function - not just call the python2 print statement > with brackets, as the semantics are different: > > $ python2 > >>> print("foo", "bar") > ('foo', 'bar') > >>> from __future__ import print_function > >>> print("foo", "bar") > foo bar > > Only the latter matches python3 semantics Thanks. Will fix in v3. >> >> Signed-off-by: Ishani Chugh <chugh.ishani@research.iiit.ac.in> >> --- >> scripts/qmp/qmp.py | 58 ++++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 37 insertions(+), 21 deletions(-) >> >> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py >> index 62d3651..58fb7d1 100644 >> --- a/scripts/qmp/qmp.py >> +++ b/scripts/qmp/qmp.py >> @@ -42,6 +42,7 @@ class QEMUMonitorProtocol: >> self.__address = address >> self._debug = debug >> self.__sock = self.__get_sock() >> + self.__data = b"" >> if server: >> self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) >> self.__sock.bind(self.__address) >> @@ -56,7 +57,7 @@ class QEMUMonitorProtocol: >> >> def __negotiate_capabilities(self): >> greeting = self.__json_read() >> - if greeting is None or not greeting.has_key('QMP'): >> + if greeting is None or 'QMP' not in greeting: >> raise QMPConnectError >> # Greeting seems ok, negotiate capabilities >> resp = self.cmd('qmp_capabilities') >> @@ -64,15 +65,28 @@ class QEMUMonitorProtocol: >> return greeting >> raise QMPCapabilitiesError >> >> + def __sock_readline(self): >> + while True: >> + ch = self.__sock.recv(1) >> + if ch is None: >> + if self.__data: >> + raise ValueError('socket closed mid-line') >> + return None >> + self.__data += ch >> + if ch == b'\n': >> + line = self.__data.decode('utf-8') >> + self.__data = b"" >> + return line >> + >> def __json_read(self, only_event=False): >> while True: >> - data = self.__sockfile.readline() >> + data = self.__sock_readline() > > So originally we could get back a "str" on python2, but now > we get a "str" (which is unicode) on python2, but "unicode" > on python2. > >> if not data: >> return >> resp = json.loads(data) > > This means the 'resp' now contains "unicode" data too on > python2 instead of 'str'. > > This hopefully isn't a problem, but we certainly need to > make sure the iotests still pass on py2, as it could well > have a ripple effect in this respect. Have you run the > iotests with this change applied ? No. I haven't run iotests with this change. Thanks for good suggestion. I will run iotests before v3. > >> if 'event' in resp: >> if self._debug: >> - print >>sys.stderr, "QMP:<<< %s" % resp >> + print("QMP:<<< %s" % resp) > > This is changing from printing to stderr, to printing to stdout > which is not desirable. Likewise all the similar changes below. > >> self.__events.append(resp) >> if not only_event: >> continue >> @@ -87,10 +101,10 @@ class QEMUMonitorProtocol: >> @param wait (bool): block until an event is available. >> @param wait (float): If wait is a float, treat it as a timeout value. >> >> - @raise QMPTimeoutError: If a timeout float is provided and the timeout >> - period elapses. >> - @raise QMPConnectError: If wait is True but no events could be >> retrieved >> - or if some other error occurred. >> + @raise QMPTimeoutError: If a timeout float is provided and the >> + timeout period elapses. >> + @raise QMPConnectError: If wait is True but no events could be >> + retrieved or if some other error occurred. >> """ > > Don't mix whitespace changes in with other functional changes. > > >> >> # Check for new events regardless and pull them into the cache: >> @@ -98,9 +112,11 @@ class QEMUMonitorProtocol: >> try: >> self.__json_read() >> except socket.error as err: >> - if err[0] == errno.EAGAIN: >> + if err.errno == errno.EAGAIN: >> # No data available >> pass >> + else: >> + raise >> self.__sock.setblocking(1) >> >> # Wait for new events, if needed. >> @@ -128,7 +144,6 @@ class QEMUMonitorProtocol: >> @raise QMPCapabilitiesError if fails to negotiate capabilities >> """ >> self.__sock.connect(self.__address) >> - self.__sockfile = self.__sock.makefile() >> if negotiate: >> return self.__negotiate_capabilities() >> >> @@ -143,7 +158,6 @@ class QEMUMonitorProtocol: >> """ >> self.__sock.settimeout(15) >> self.__sock, _ = self.__sock.accept() >> - self.__sockfile = self.__sock.makefile() >> return self.__negotiate_capabilities() >> >> def cmd_obj(self, qmp_cmd): >> @@ -155,16 +169,17 @@ class QEMUMonitorProtocol: >> been closed >> """ >> if self._debug: >> - print >>sys.stderr, "QMP:>>> %s" % qmp_cmd >> + print("QMP:>>> %s" % qmp_cmd) >> try: >> - self.__sock.sendall(json.dumps(qmp_cmd)) >> + command = json.dumps(qmp_cmd) >> + self.__sock.sendall(command.encode('UTF-8')) >> except socket.error as err: >> - if err[0] == errno.EPIPE: >> + if err.errno == errno.EPIPE: >> return >> - raise socket.error(err) >> + raise >> resp = self.__json_read() >> if self._debug: >> - print >>sys.stderr, "QMP:<<< %s" % resp >> + print("QMP:<<< %s" % resp) >> return resp >> >> def cmd(self, name, args=None, id=None): >> @@ -175,7 +190,7 @@ class QEMUMonitorProtocol: >> @param args: command arguments (dict) >> @param id: command id (dict, list, string or int) >> """ >> - qmp_cmd = { 'execute': name } >> + qmp_cmd = {'execute': name} > > Again bogus whitespace changes > >> if args: >> qmp_cmd['arguments'] = args >> if id: >> @@ -184,7 +199,7 @@ class QEMUMonitorProtocol: >> >> def command(self, cmd, **kwds): >> ret = self.cmd(cmd, kwds) >> - if ret.has_key('error'): >> + if 'error' in ret: >> raise Exception(ret['error']['desc']) >> return ret['return'] >> >> @@ -197,8 +212,8 @@ class QEMUMonitorProtocol: >> >> @raise QMPTimeoutError: If a timeout float is provided and the timeout >> period elapses. >> - @raise QMPConnectError: If wait is True but no events could be >> retrieved >> - or if some other error occurred. >> + @raise QMPConnectError: If wait is True but no events could be >> + retrieved or if some other error occurred. > > And again > >> >> @return The first available QMP event, or None. >> """ >> @@ -217,8 +232,8 @@ class QEMUMonitorProtocol: >> >> @raise QMPTimeoutError: If a timeout float is provided and the timeout >> period elapses. >> - @raise QMPConnectError: If wait is True but no events could be >> retrieved >> - or if some other error occurred. >> + @raise QMPConnectError: If wait is True but no events could be >> + retrieved or if some other error occurred. > > Same > >> >> @return The list of available QMP events. >> """ >> @@ -245,3 +260,4 @@ class QEMUMonitorProtocol: >> >> def is_scm_available(self): >> return self.__sock.family == socket.AF_UNIX >> + > > Adding trailing blank line to the file is bad. The reason for making formatting changes is adhering to PEP8 standards. I will undo these changes in v3 and send a separate patch for PEP8 standards. > Regards, > Daniel > -- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| Regards, Ishani
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py index 62d3651..58fb7d1 100644 --- a/scripts/qmp/qmp.py +++ b/scripts/qmp/qmp.py @@ -42,6 +42,7 @@ class QEMUMonitorProtocol: self.__address = address self._debug = debug self.__sock = self.__get_sock() + self.__data = b"" if server: self.__sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) self.__sock.bind(self.__address) @@ -56,7 +57,7 @@ class QEMUMonitorProtocol: def __negotiate_capabilities(self): greeting = self.__json_read() - if greeting is None or not greeting.has_key('QMP'): + if greeting is None or 'QMP' not in greeting: raise QMPConnectError # Greeting seems ok, negotiate capabilities resp = self.cmd('qmp_capabilities') @@ -64,15 +65,28 @@ class QEMUMonitorProtocol: return greeting raise QMPCapabilitiesError + def __sock_readline(self): + while True: + ch = self.__sock.recv(1) + if ch is None: + if self.__data: + raise ValueError('socket closed mid-line') + return None + self.__data += ch + if ch == b'\n': + line = self.__data.decode('utf-8') + self.__data = b"" + return line + def __json_read(self, only_event=False): while True: - data = self.__sockfile.readline() + data = self.__sock_readline() if not data: return resp = json.loads(data) if 'event' in resp: if self._debug: - print >>sys.stderr, "QMP:<<< %s" % resp + print("QMP:<<< %s" % resp) self.__events.append(resp) if not only_event: continue @@ -87,10 +101,10 @@ class QEMUMonitorProtocol: @param wait (bool): block until an event is available. @param wait (float): If wait is a float, treat it as a timeout value. - @raise QMPTimeoutError: If a timeout float is provided and the timeout - period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPTimeoutError: If a timeout float is provided and the + timeout period elapses. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. """ # Check for new events regardless and pull them into the cache: @@ -98,9 +112,11 @@ class QEMUMonitorProtocol: try: self.__json_read() except socket.error as err: - if err[0] == errno.EAGAIN: + if err.errno == errno.EAGAIN: # No data available pass + else: + raise self.__sock.setblocking(1) # Wait for new events, if needed. @@ -128,7 +144,6 @@ class QEMUMonitorProtocol: @raise QMPCapabilitiesError if fails to negotiate capabilities """ self.__sock.connect(self.__address) - self.__sockfile = self.__sock.makefile() if negotiate: return self.__negotiate_capabilities() @@ -143,7 +158,6 @@ class QEMUMonitorProtocol: """ self.__sock.settimeout(15) self.__sock, _ = self.__sock.accept() - self.__sockfile = self.__sock.makefile() return self.__negotiate_capabilities() def cmd_obj(self, qmp_cmd): @@ -155,16 +169,17 @@ class QEMUMonitorProtocol: been closed """ if self._debug: - print >>sys.stderr, "QMP:>>> %s" % qmp_cmd + print("QMP:>>> %s" % qmp_cmd) try: - self.__sock.sendall(json.dumps(qmp_cmd)) + command = json.dumps(qmp_cmd) + self.__sock.sendall(command.encode('UTF-8')) except socket.error as err: - if err[0] == errno.EPIPE: + if err.errno == errno.EPIPE: return - raise socket.error(err) + raise resp = self.__json_read() if self._debug: - print >>sys.stderr, "QMP:<<< %s" % resp + print("QMP:<<< %s" % resp) return resp def cmd(self, name, args=None, id=None): @@ -175,7 +190,7 @@ class QEMUMonitorProtocol: @param args: command arguments (dict) @param id: command id (dict, list, string or int) """ - qmp_cmd = { 'execute': name } + qmp_cmd = {'execute': name} if args: qmp_cmd['arguments'] = args if id: @@ -184,7 +199,7 @@ class QEMUMonitorProtocol: def command(self, cmd, **kwds): ret = self.cmd(cmd, kwds) - if ret.has_key('error'): + if 'error' in ret: raise Exception(ret['error']['desc']) return ret['return'] @@ -197,8 +212,8 @@ class QEMUMonitorProtocol: @raise QMPTimeoutError: If a timeout float is provided and the timeout period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. @return The first available QMP event, or None. """ @@ -217,8 +232,8 @@ class QEMUMonitorProtocol: @raise QMPTimeoutError: If a timeout float is provided and the timeout period elapses. - @raise QMPConnectError: If wait is True but no events could be retrieved - or if some other error occurred. + @raise QMPConnectError: If wait is True but no events could be + retrieved or if some other error occurred. @return The list of available QMP events. """ @@ -245,3 +260,4 @@ class QEMUMonitorProtocol: def is_scm_available(self): return self.__sock.family == socket.AF_UNIX +
This patch intends to make qmp.py compatible with both python2 and python3. * Python 3 does not have dict.has_key(key), use key in dict instead * Avoid line-based I/O since Python 2/3 have different character encoding behavior. Explicitly encode/decode JSON UTF-8. * Replace print by print function. Signed-off-by: Ishani Chugh <chugh.ishani@research.iiit.ac.in> --- scripts/qmp/qmp.py | 58 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 21 deletions(-)