On Mon, Mar 13, 2017 at 8:04 AM Markus Armbruster <arm...@redhat.com> wrote:
> Nir Soffer <nir...@gmail.com> writes: > > > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <arm...@redhat.com> > wrote: > >> John Snow <js...@redhat.com> writes: > >> > >>> On 03/07/2017 03:16 AM, Markus Armbruster wrote: > >>>> John Snow <js...@redhat.com> writes: > >>>> > >>>>> 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...!) > >>>> > >>>> Hah! Saving it would be easy enough, but reloading it... okay, call > it > >>>> a "backup" and declare victory when saving works. > >>>> > >>>>>>>>>> > >>>>>>>>>> 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. > >>>> > >>>> For what it's worth, bash defaults HISTSIZE to 500. > >>>> > >>>> GNU Readline lets users configure it in ~/.inputrc. Conditional > >>>> configuration is possible, i.e. something like > >>>> > >>>> $if Qmp-shell > >>>> set history-size 5000 > >>>> $endif > >>>> > >>>> should work, provided qmp-shell sets rl_readline_name as it should. > >>>> > >>> > >>> Spoke too soon. I don't see a way to control this in python's readline > >>> library... I'm not very familiar with readline, is there some > >>> environment variable or something perhaps? > >>> (It looks like python's code just hard-sets it to "python" ...) > >> > >> How sad. If https://bugs.python.org/ didn't require a login, I > would've > >> filed a bug already. > >> > >> I'm afraid all I can offer meanwhile is INPUTRC: > >> > >> Any user can customize programs that use Readline by putting > >> commands in an "inputrc" file, conventionally in his home directory. > >> The name of this file is taken from the value of the environment > >> variable `INPUTRC'. If that variable is unset, the default is > >> `~/.inputrc'. If that file does not exist or cannot be read, the > >> ultimate default is `/etc/inputrc'. > >> > > > > Having a way to limit history globally looks good enough. > > Certainly good enough for merging qmp-shell patches. > > > Note that python readline does not report the readline > > limit correctly (get_history_length() returns -1), but saving > > history uses the limit defined in .inputrc. > > > > Playing with this, I found a nice bug - if you set history > > size to N, and your history file contains 2 * N items or more, > > python segfaults when entering the first input line. > > > > I'll file a python bug later. > > Please do. > Reported here: http://bugs.python.org/issue29854 https://github.com/python/cpython/pull/728 If you could throw in a (separate) request for letting us set > rl_readline_name, that would be great. > Will do. > > [...] >