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

[Nir Soffer <nirsof@gmail.com>]

On Wed, Mar 1, 2017 at 9:44 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>
> ---
>  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

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

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.

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

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?

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