On 03/06/2017 03:18 AM, Markus Armbruster wrote: > Nir Soffer <nir...@gmail.com> writes: > >> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <js...@redhat.com> wrote: >>> >>> >>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>> On Fri, Mar 3, 2017 at 8:54 PM, John Snow <js...@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 <js...@redhat.com> >>>>> --- >>>>> >>>>> v2: Adjusted the errors to whine about non-ENOENT errors, but still >>>>> intercept all errors as non-fatal. >>>>> Save history atexit() to match bash standard behavior >>>>> >>>>> scripts/qmp/qmp-shell | 19 +++++++++++++++++++ >>>>> 1 file changed, 19 insertions(+) >>>>> >>>>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell >>>>> index 0373b24..55a8285 100755 >>>>> --- a/scripts/qmp/qmp-shell >>>>> +++ b/scripts/qmp/qmp-shell >>>>> @@ -70,6 +70,9 @@ import json >>>>> import ast >>>>> import readline >>>>> import sys >>>>> +import os >>>>> +import errno >>>>> +import atexit >>>>> >>>>> class QMPCompleter(list): >>>>> def complete(self, text, state): >>>>> @@ -109,6 +112,7 @@ class QMPShell(qmp.QEMUMonitorProtocol): >>>>> self._pretty = pretty >>>>> self._transmode = False >>>>> self._actions = list() >>>>> + self._histfile = os.path.join(os.path.expanduser('~'), >>>>> '.qmp_history') > > I selfishly object to this filename, because I'm using it with > > $ socat UNIX:/work/armbru/images/test-qmp > READLINE,history=$HOME/.qmp_history,prompt='QMP> ' > > Just kidding. But seriously, shouldn't this be named after the > *application* (qmp-shell) rather than the protocol (qmp)? >
Seeing as the history itself is the qmp-shell syntax, you are correct. (Hey, it would be interesting to store the generated QMP into the qmp_history, though...!) >>>>> >>>>> def __get_address(self, arg): >>>>> """ >>>>> @@ -137,6 +141,21 @@ 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 Exception as e: >>>>> + if isinstance(e, IOError) and e.errno == errno.ENOENT: >>>>> + # File not found. No problem. >>>>> + pass >>>>> + else: >>>>> + print "Failed to read history '%s'; %s" % >>>>> (self._histfile, e) >>>> >>>> I would handle only IOError, since any other error means a bug in this code >>>> or in the underlying readline library, and the best way to handle this is >>>> to >>>> let it fail loudly. >>>> >>> >>> Disagree. No reason to stop the shell from working because a QOL feature >>> didn't initialize correctly. >>> >>> The warning will be enough to solicit reports and fixes if necessary. >> >> I agree, the current solution is good tradeoff. > > For what it's worth, bash seems to silently ignore a history file it > can't read. Tested by running "HISTFILE=xxx bash", then chmod 0 xxx, da > capo. > Right, done by example. >> One thing missing, is a call to readline.set_history_length, without >> it the history >> will grow without limit, see: >> https://docs.python.org/2/library/readline.html#readline.set_history_length > > Should this be addressed for 2.9? > You can add a limit if you want to; I don't have suggestions for which completely arbitrary limit makes sense, so I left it blank intentionally. >>>>> + atexit.register(self.__save_history) >>>>> + >>>>> + def __save_history(self): >>>>> + try: >>>>> + readline.write_history_file(self._histfile) >>>>> + except Exception as e: >>>>> + print "Failed to save history file '%s'; %s" % >>>>> (self._histfile, e) >>>>> >>>>> def __parse_value(self, val): >>>>> try: >>>> >>>> But I think this is good enough and useful as is. >>>> >>>> Reviewed-by: Nir Soffer <nir...@gmail.com> >>>>