On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes:
>
>> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
>>> John Snow <js...@redhat.com> writes:
>>>
>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>>>> Nir Soffer <nir...@gmail.com> writes:
>>>>>
>>>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <js...@redhat.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote:
>>>>>>>> On Fri, Mar 3, 2017 at 8:54 PM, John Snow <js...@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 <js...@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)?
>>>>>
>>>>
>>>> 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.
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.
>>>>>>>>> + 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 <nir...@gmail.com>
>>>>>>>>
