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

[Markus Armbruster]

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.

If you could throw in a (separate) request for letting us set
rl_readline_name, that would be great.

[...]