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

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

[John Snow]

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

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

[Nir Soffer]

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>

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

[John Snow]

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

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

[Nir Soffer]

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

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

[Markus Armbruster]

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

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

[Kashyap Chamarthy]

On Mon, Mar 06, 2017 at 09:18:29AM +0100, Markus Armbruster wrote:
> Nir Soffer <nirsof@gmail.com> writes:
> 
> > On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote:

[...]

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

FWIW, that sounds reasonable to me.  It's clearer that way -- the raw
QMP input history, when using via `socat` as above and the 'qmp-shell'
history won't get mixed up.

[...]

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

Sounds like a nice-to-have for me.

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

-- 
/kashyap

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

[John Snow]

On 03/06/2017 03:18 AM, Markus Armbruster wrote:
> 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)?
> 

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

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

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>> 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)?
>> 
>
> 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.

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

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

[John Snow]

On 03/07/2017 03:16 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>> 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)?
>>>
>>
>> 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.
> 

There's the winning ticket!

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

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

[John Snow]

On 03/07/2017 03:16 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>> 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)?
>>>
>>
>> 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" ...)

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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>>> 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)?
>>>>
>>>
>>> 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'.

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

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

[Nir Soffer]

On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com> wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>>>> 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)?
>>>>>
>>>>
>>>> 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 <nirsof@gmail.com>
>>>>>>>>

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

[Markus Armbruster]

Nir Soffer <nirsof@gmail.com> writes:

> On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
>>>>>> 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)?
>>>>>>
>>>>>
>>>>> 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.

Certainly good enough for merging qmp-shell patches.

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

Please do.

If you could throw in a (separate) request for letting us set
rl_readline_name, that would be great.

[...]

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

[Nir Soffer]

On Mon, Mar 13, 2017 at 8:04 AM Markus Armbruster <armbru@redhat.com> wrote:

> Nir Soffer <nirsof@gmail.com> writes:
>
> > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com>
> wrote:
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >>> On 03/07/2017 03:16 AM, Markus Armbruster wrote:
> >>>> John Snow <jsnow@redhat.com> writes:
> >>>>
> >>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote:
> >>>>>> 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)?
> >>>>>>
> >>>>>
> >>>>> 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.
>
> Certainly good enough for merging qmp-shell patches.
>
> > 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.
>
> Please do.
>

Reported here:
http://bugs.python.org/issue29854
https://github.com/python/cpython/pull/728

If you could throw in a (separate) request for letting us set
> rl_readline_name, that would be great.
>

Will do.


>
> [...]
>