On Thu, Mar 2, 2017 at 12:19 AM, John Snow <js...@redhat.com> wrote:
>
>
> On 03/01/2017 05:01 PM, Nir Soffer wrote:
>> On Wed, Mar 1, 2017 at 9:44 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>
>>> ---
>>> scripts/qmp/qmp-shell | 14 ++++++++++++++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/scripts/qmp/qmp-shell b/scripts/qmp/qmp-shell
>>> index 0373b24..b19f44b 100755
>>> --- a/scripts/qmp/qmp-shell
>>> +++ b/scripts/qmp/qmp-shell
>>> @@ -70,6 +70,7 @@ import json
>>> import ast
>>> import readline
>>> import sys
>>> +import os
>>>
>>> class QMPCompleter(list):
>>> def complete(self, text, state):
>>> @@ -109,6 +110,8 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>> self._pretty = pretty
>>> self._transmode = False
>>> self._actions = list()
>>> + self._histfile = os.path.join(os.path.expanduser('~'),
>>> + '.qmp_history')
>>
>> This can be little bit more readable in one line
>>
>
> I thought I was over 80, but maybe not.
>
>>>
>>> def __get_address(self, arg):
>>> """
>>> @@ -137,6 +140,16 @@ 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:
>>> + pass
>>
>> This hides all errors, including KeyboardInterrupt and SystemExit, and will
>> make debugging impossible.
>>
>
> Indeed, I want to ignore errors related to a missing history file. It
> wasn't documented, and this isn't an important feature (for a shell
> script only used for debugging), so I went with the dumb thing.
>
>> It looks like you want to ignore missing history file, but this way we also
>> ignore permission error or even typo in the code. For example this will
>> fail silently:
>>
>> try:
>> readdline.read_history_file(self._histfile)
>> except:
>> pass
>>
>> The docs do not specify the possible errors, but the code is raising IOError:
>> https://github.com/python/cpython/blob/2.7/Modules/readline.c#L126
>>
>> So it would be best to handle only IOError, and ignore ENOENT. Any other
>> error should fail in a visible way.
>>
>
> Maybe not "fail," but perhaps "warn." This feature is not so important
> that it should inhibit normal operation.
Yes, warning with the text from the IOError should be best.
>
>>> +
>>> + def __save_history(self):
>>> + try:
>>> + readline.write_history_file(self._histfile)
>>> + except:
>>> + pass
>>
>> Same, but I'm not sure what errors should be ignored. Do we want to silently
>> ignore a read only file system? no space?
>>
>
> Pretty much my thought, yes. I could "warn" on the first failure and
> then stifle subsequent ones. I don't want this to be an irritant.
>
>> I think a safe way would be to print a warning if the history file
>> cannot be saved
>> with the text from the IOError.
>>
>>>
>>> def __parse_value(self, val):
>>> try:
>>> @@ -244,6 +257,7 @@ class QMPShell(qmp.QEMUMonitorProtocol):
>>> print 'command format: <command-name> ',
>>> print '[arg-name1=arg1] ... [arg-nameN=argN]'
>>> return True
>>> + self.__save_history()
>>
>> This will save the history after every command, making error handling
>> more complicated, and also unneeded, since we don't care about history
>> if you kill the qmp-shell process, right?
>>
>
> I suppose so. My thought was more along the lines of: "If the program
> explodes, I'd like to have the intervening history saved."
Python programs do not explode in this way usually.
> I didn't
> think this would complicate performance of a debugging tool.
>
> Why do you feel this would make error handling more complicated?
Because we have to handle errors on each command, instead of once
during exit.
> Why do you think we wouldn't care about the history if we kill the
> qmp-shell process?
We care about the history, but do you expect that the program will not
handle SIGTERM properly often?
>> We can invoke readline.write_history_file() using atexit. This is also
>> what the docs suggest, see:
>> https://docs.python.org/2/library/readline.html#example
>>
>> Nir
>>
>>> # For transaction mode, we may have just cached the action:
>>> if qmpcmd is None:
>>> return True
>>> --
>>> 2.9.3
>>>
>>>
