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

From: Nir Soffer <nirsof@gmail.com>
To: John Snow <jsnow@redhat.com>
Cc: qemu-devel@nongnu.org, armbru@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
Date: Fri, 3 Mar 2017 21:26:22 +0200	[thread overview]
Message-ID: <CAMr-obuiwE_ef6OPSJbBtmZYsaqo_mzU9E6q8zhbrviZ6_=krQ@mail.gmail.com> (raw)
In-Reply-To: <20170303185427.32681-1-jsnow@redhat.com>

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>