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

[John Snow]

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...!)

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

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