| Message ID | 20170301194433.23379-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 1, 2017 at 9:44 PM, John Snow <jsnow@redhat.com> wrote: > > Use the existing readline history function we are utilizing > to provide persistent command history across instances of qmp-shell. > > This assists entering debug commands across sessions that may be > interrupted by QEMU sessions terminating, where the qmp-shell has > to be relaunched. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qmp/qmp-shell | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell > index 0373b24..b19f44b 100755 > --- a/scripts/qmp/qmp-shell > +++ b/scripts/qmp/qmp-shell > @@ -70,6 +70,7 @@ import json > import ast > import readline > import sys > +import os > > class QMPCompleter(list): > def complete(self, text, state): > @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol): > self._pretty = pretty > self._transmode = False > self._actions = list() > + self._histfile = os.path.join(os.path.expanduser('~'), > + '.qmp_history') This can be little bit more readable in one line > > def __get_address(self, arg): > """ > @@ -137,6 +140,16 @@ class QMPShell(qmp.QEMUMonitorProtocol): > # XXX: default delimiters conflict with some command names (eg. query-), > # clearing everything as it doesn't seem to matter > readline.set_completer_delims('') > + try: > + readline.read_history_file(self._histfile) > + except: > + pass This hides all errors, including KeyboardInterrupt and SystemExit, and will make debugging impossible. It looks like you want to ignore missing history file, but this way we also ignore permission error or even typo in the code. For example this will fail silently: try: readdline.read_history_file(self._histfile) except: pass The docs do not specify the possible errors, but the code is raising IOError: https://github.com/python/cpython/blob/2.7/Modules/readline.c#L126 So it would be best to handle only IOError, and ignore ENOENT. Any other error should fail in a visible way. > + > + def __save_history(self): > + try: > + readline.write_history_file(self._histfile) > + except: > + pass Same, but I'm not sure what errors should be ignored. Do we want to silently ignore a read only file system? no space? I think a safe way would be to print a warning if the history file cannot be saved with the text from the IOError. > > def __parse_value(self, val): > try: > @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): > print 'command format: <command-name> ', > print '[arg-name1=arg1] ... [arg-nameN=argN]' > return True > + self.__save_history() This will save the history after every command, making error handling more complicated, and also unneeded, since we don't care about history if you kill the qmp-shell process, right? We can invoke readline.write_history_file() using atexit. This is also what the docs suggest, see: https://docs.python.org/2/library/readline.html#example Nir > # For transaction mode, we may have just cached the action: > if qmpcmd is None: > return True > -- > 2.9.3 > >
On 03/01/2017 05:01 PM, Nir Soffer wrote: > On Wed, Mar 1, 2017 at 9:44 PM, John Snow <jsnow@redhat.com> wrote: >> >> Use the existing readline history function we are utilizing >> to provide persistent command history across instances of qmp-shell. >> >> This assists entering debug commands across sessions that may be >> interrupted by QEMU sessions terminating, where the qmp-shell has >> to be relaunched. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qmp/qmp-shell | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >> index 0373b24..b19f44b 100755 >> --- a/scripts/qmp/qmp-shell >> +++ b/scripts/qmp/qmp-shell >> @@ -70,6 +70,7 @@ import json >> import ast >> import readline >> import sys >> +import os >> >> class QMPCompleter(list): >> def complete(self, text, state): >> @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> self._pretty = pretty >> self._transmode = False >> self._actions = list() >> + self._histfile = os.path.join(os.path.expanduser('~'), >> + '.qmp_history') > > This can be little bit more readable in one line > I thought I was over 80, but maybe not. >> >> def __get_address(self, arg): >> """ >> @@ -137,6 +140,16 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> # XXX: default delimiters conflict with some command names (eg. query-), >> # clearing everything as it doesn't seem to matter >> readline.set_completer_delims('') >> + try: >> + readline.read_history_file(self._histfile) >> + except: >> + pass > > This hides all errors, including KeyboardInterrupt and SystemExit, and will > make debugging impossible. > Indeed, I want to ignore errors related to a missing history file. It wasn't documented, and this isn't an important feature (for a shell script only used for debugging), so I went with the dumb thing. > It looks like you want to ignore missing history file, but this way we also > ignore permission error or even typo in the code. For example this will > fail silently: > > try: > readdline.read_history_file(self._histfile) > except: > pass > > The docs do not specify the possible errors, but the code is raising IOError: > https://github.com/python/cpython/blob/2.7/Modules/readline.c#L126 > > So it would be best to handle only IOError, and ignore ENOENT. Any other > error should fail in a visible way. > Maybe not "fail," but perhaps "warn." This feature is not so important that it should inhibit normal operation. >> + >> + def __save_history(self): >> + try: >> + readline.write_history_file(self._histfile) >> + except: >> + pass > > Same, but I'm not sure what errors should be ignored. Do we want to silently > ignore a read only file system? no space? > Pretty much my thought, yes. I could "warn" on the first failure and then stifle subsequent ones. I don't want this to be an irritant. > I think a safe way would be to print a warning if the history file > cannot be saved > with the text from the IOError. > >> >> def __parse_value(self, val): >> try: >> @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): >> print 'command format: <command-name> ', >> print '[arg-name1=arg1] ... [arg-nameN=argN]' >> return True >> + self.__save_history() > > This will save the history after every command, making error handling > more complicated, and also unneeded, since we don't care about history > if you kill the qmp-shell process, right? > I suppose so. My thought was more along the lines of: "If the program explodes, I'd like to have the intervening history saved." I didn't think this would complicate performance of a debugging tool. Why do you feel this would make error handling more complicated? Why do you think we wouldn't care about the history if we kill the qmp-shell process? > We can invoke readline.write_history_file() using atexit. This is also > what the docs suggest, see: > https://docs.python.org/2/library/readline.html#example > > Nir > >> # For transaction mode, we may have just cached the action: >> if qmpcmd is None: >> return True >> -- >> 2.9.3 >> >>
On Thu, Mar 2, 2017 at 12:19 AM, John Snow <jsnow@redhat.com> wrote: > > > On 03/01/2017 05:01 PM, Nir Soffer wrote: >> On Wed, Mar 1, 2017 at 9:44 PM, John Snow <jsnow@redhat.com> wrote: >>> >>> Use the existing readline history function we are utilizing >>> to provide persistent command history across instances of qmp-shell. >>> >>> This assists entering debug commands across sessions that may be >>> interrupted by QEMU sessions terminating, where the qmp-shell has >>> to be relaunched. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> scripts/qmp/qmp-shell | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >>> index 0373b24..b19f44b 100755 >>> --- a/scripts/qmp/qmp-shell >>> +++ b/scripts/qmp/qmp-shell >>> @@ -70,6 +70,7 @@ import json >>> import ast >>> import readline >>> import sys >>> +import os >>> >>> class QMPCompleter(list): >>> def complete(self, text, state): >>> @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol): >>> self._pretty = pretty >>> self._transmode = False >>> self._actions = list() >>> + self._histfile = os.path.join(os.path.expanduser('~'), >>> + '.qmp_history') >> >> This can be little bit more readable in one line >> > > I thought I was over 80, but maybe not. > >>> >>> def __get_address(self, arg): >>> """ >>> @@ -137,6 +140,16 @@ class QMPShell(qmp.QEMUMonitorProtocol): >>> # XXX: default delimiters conflict with some command names (eg. query-), >>> # clearing everything as it doesn't seem to matter >>> readline.set_completer_delims('') >>> + try: >>> + readline.read_history_file(self._histfile) >>> + except: >>> + pass >> >> This hides all errors, including KeyboardInterrupt and SystemExit, and will >> make debugging impossible. >> > > Indeed, I want to ignore errors related to a missing history file. It > wasn't documented, and this isn't an important feature (for a shell > script only used for debugging), so I went with the dumb thing. > >> It looks like you want to ignore missing history file, but this way we also >> ignore permission error or even typo in the code. For example this will >> fail silently: >> >> try: >> readdline.read_history_file(self._histfile) >> except: >> pass >> >> The docs do not specify the possible errors, but the code is raising IOError: >> https://github.com/python/cpython/blob/2.7/Modules/readline.c#L126 >> >> So it would be best to handle only IOError, and ignore ENOENT. Any other >> error should fail in a visible way. >> > > Maybe not "fail," but perhaps "warn." This feature is not so important > that it should inhibit normal operation. Yes, warning with the text from the IOError should be best. > >>> + >>> + def __save_history(self): >>> + try: >>> + readline.write_history_file(self._histfile) >>> + except: >>> + pass >> >> Same, but I'm not sure what errors should be ignored. Do we want to silently >> ignore a read only file system? no space? >> > > Pretty much my thought, yes. I could "warn" on the first failure and > then stifle subsequent ones. I don't want this to be an irritant. > >> I think a safe way would be to print a warning if the history file >> cannot be saved >> with the text from the IOError. >> >>> >>> def __parse_value(self, val): >>> try: >>> @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): >>> print 'command format: <command-name> ', >>> print '[arg-name1=arg1] ... [arg-nameN=argN]' >>> return True >>> + self.__save_history() >> >> This will save the history after every command, making error handling >> more complicated, and also unneeded, since we don't care about history >> if you kill the qmp-shell process, right? >> > > I suppose so. My thought was more along the lines of: "If the program > explodes, I'd like to have the intervening history saved." Python programs do not explode in this way usually. > I didn't > think this would complicate performance of a debugging tool. > > Why do you feel this would make error handling more complicated? Because we have to handle errors on each command, instead of once during exit. > Why do you think we wouldn't care about the history if we kill the > qmp-shell process? We care about the history, but do you expect that the program will not handle SIGTERM properly often? >> We can invoke readline.write_history_file() using atexit. This is also >> what the docs suggest, see: >> https://docs.python.org/2/library/readline.html#example >> >> Nir >> >>> # For transaction mode, we may have just cached the action: >>> if qmpcmd is None: >>> return True >>> -- >>> 2.9.3 >>> >>>
On Wed, Mar 01, 2017 at 02:44:33PM -0500, John Snow wrote: > Use the existing readline history function we are utilizing > to provide persistent command history across instances of qmp-shell. Thanks for adding it to 'qmp-shell' this is useful. I normally use the READLINE functionality using 'qmp_history' file in conjunction with `socat` (thanks: Markus Armbruster). $ socat UNIX:./qmp-sock READLINE,history=$HOME/.qmp_history,prompt='QMP> ' As I often shuffle between 'qmp-shell' and `socat`, so having the history file conveniently track history is very handy. > This assists entering debug commands across sessions that may be > interrupted by QEMU sessions terminating, where the qmp-shell has > to be relaunched. > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qmp/qmp-shell | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) I just tested this patch. And the bare-bones functionality works fine for me: if the 'qmp_history' file already exists, it'll just append the history to it; otherwise, it'll create one. And I have the same question as you, to Nir (especially, I do care about the history if the 'qmp-shell' process is killed). For now, I'm abivalent about handling ENOSPC. So, with or without the error handling changes that you're discussing, I'm fine with this initial version of the change, as it is additive. Just for posterity, my test 'evidence' - Apply the patch to current Git, and compile: $ git log --oneline | head -1 b8c1c7b qmp-shell: add persistent command history $ ./qemu-system-x86_64 -version QEMU emulator version 2.8.50 (v2.8.0-1813-gb8c1c7b-dirty) - Run a minimal QEMU instance with '-qmp unix:./qmp-sock,server,nowait'] - Run a few commands from the QMP shell: $ ./qmp-shell -v -p /export/qmp-sock - Observe the history file: $ tail ~/.qmp_history } { "execute":"nbd-server-stop" } { "execute": "query-block" } query-block query-name query-named-block-nodes query-kvm quit You see both the raw QMP JSON and 'qmp-shell' history there (as I mentioned I shuffle between both). So, FWIW: Tested-by: Kashyap Chamarthy <kchamart@redhat.com> > diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell > index 0373b24..b19f44b 100755 > --- a/scripts/qmp/qmp-shell > +++ b/scripts/qmp/qmp-shell > @@ -70,6 +70,7 @@ import json > import ast > import readline > import sys > +import os > > class QMPCompleter(list): > def complete(self, text, state): > @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol): > self._pretty = pretty > self._transmode = False > self._actions = list() > + self._histfile = os.path.join(os.path.expanduser('~'), > + '.qmp_history') > > def __get_address(self, arg): > """ > @@ -137,6 +140,16 @@ class QMPShell(qmp.QEMUMonitorProtocol): > # XXX: default delimiters conflict with some command names (eg. query-), > # clearing everything as it doesn't seem to matter > readline.set_completer_delims('') > + try: > + readline.read_history_file(self._histfile) > + except: > + pass > + > + def __save_history(self): > + try: > + readline.write_history_file(self._histfile) > + except: > + pass > > def __parse_value(self, val): > try: > @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): > print 'command format: <command-name> ', > print '[arg-name1=arg1] ... [arg-nameN=argN]' > return True > + self.__save_history() > # For transaction mode, we may have just cached the action: > if qmpcmd is None: > return True > -- > 2.9.3 >
diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell index 0373b24..b19f44b 100755 --- a/scripts/qmp/qmp-shell +++ b/scripts/qmp/qmp-shell @@ -70,6 +70,7 @@ import json import ast import readline import sys +import os class QMPCompleter(list): def complete(self, text, state): @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol): self._pretty = pretty self._transmode = False self._actions = list() + self._histfile = os.path.join(os.path.expanduser('~'), + '.qmp_history') def __get_address(self, arg): """ @@ -137,6 +140,16 @@ class QMPShell(qmp.QEMUMonitorProtocol): # XXX: default delimiters conflict with some command names (eg. query-), # clearing everything as it doesn't seem to matter readline.set_completer_delims('') + try: + readline.read_history_file(self._histfile) + except: + pass + + def __save_history(self): + try: + readline.write_history_file(self._histfile) + except: + pass def __parse_value(self, val): try: @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): print 'command format: <command-name> ', print '[arg-name1=arg1] ... [arg-nameN=argN]' return True + self.__save_history() # For transaction mode, we may have just cached the action: if qmpcmd is None: return True
Use the existing readline history function we are utilizing to provide persistent command history across instances of qmp-shell. This assists entering debug commands across sessions that may be interrupted by QEMU sessions terminating, where the qmp-shell has to be relaunched. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qmp/qmp-shell | 14 ++++++++++++++ 1 file changed, 14 insertions(+)