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

[Markus Armbruster]

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'.

>>>>>>>> +        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 <address@hidden>
>>>>>>>