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

From: Markus Armbruster <armbru@redhat.com>
To: Nir Soffer <nirsof@gmail.com>
Cc: John Snow <jsnow@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
Date: Mon, 06 Mar 2017 09:18:29 +0100	[thread overview]
Message-ID: <871sua4y3u.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <CAMr-obsrUqvqbLabYgKgv1J+ztn5XCwto6LowoN=S0ZWuFLZMA@mail.gmail.com> (Nir Soffer's message of "Sat, 4 Mar 2017 21:47:19 +0200")

Nir Soffer <nirsof@gmail.com> writes:

> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote:
>>
>>
>> On 03/03/2017 02:26 PM, Nir Soffer wrote:
>>> On Fri, Mar 3, 2017 at 8:54 PM, John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>>>> ---
>>>>
>>>> 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)?

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

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

>>>> +        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 <nirsof@gmail.com>
>>>