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 <address@hidden> writes:
> 
> > On Fri, Mar 3, 2017 at 9:29 PM, John Snow <address@hidden> 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 <address@hidden>
> >>>
> 

-- 
/kashyap