On Mon, Mar 06, 2017 at 09:18:29AM +0100, Markus Armbruster wrote:
> Nir Soffer <nir...@gmail.com> writes:
>
> > On Fri, Mar 3, 2017 at 9:29 PM, John Snow <js...@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 <nir...@gmail.com>
> >>>
>
--
/kashyap
