Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history

[Nir Soffer]

On Mon, Mar 13, 2017 at 8:04 AM Markus Armbruster <address@hidden> wrote:

> Nir Soffer <address@hidden> writes:
>
> > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <address@hidden>
> wrote:
> >> John Snow <address@hidden> writes:
> >>
> >>> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
> >>>> John Snow <address@hidden> writes:
> >>>>
> >>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
> >>>>>> Nir Soffer <address@hidden> writes:
> >>>>>>
> >>>>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <address@hidden>
> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote:
> >>>>>>>>> On Fri, Mar 3, 2017 at 8:54 PM, John Snow <address@hidden>
> 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 <address@hidden>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>> 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.


>
> [...]
>