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

[John Snow]

On 03/01/2017 05:01 PM, Nir Soffer wrote:
> On Wed, Mar 1, 2017 at 9:44 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>
>> ---
>>  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.

>> +
>> +    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." I didn't
think this would complicate performance of a debugging tool.

Why do you feel this would make error handling more complicated?

Why do you think we wouldn't care about the history if we kill the
qmp-shell process?

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