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

[Markus Armbruster]

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.

>>>>>> +        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>
>>>>>