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')
 
     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)
+        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:
-- 
2.9.3

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

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

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

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

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

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

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