From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1clKxM-0005gg-W2 for qemu-devel@nongnu.org; Tue, 07 Mar 2017 14:39:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1clKxI-0007rM-7G for qemu-devel@nongnu.org; Tue, 07 Mar 2017 14:39:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36018) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1clKxH-0007qs-UJ for qemu-devel@nongnu.org; Tue, 07 Mar 2017 14:39:32 -0500 References: <20170303185427.32681-1-jsnow@redhat.com> <5303cac4-a8d8-bf89-b525-3b0fb1b19127@redhat.com> <871sua4y3u.fsf@dusky.pond.sub.org> <3fbdfabf-82b4-d774-d5f4-2c50a6d740dc@redhat.com> <87efy9ijrf.fsf@dusky.pond.sub.org> From: John Snow Message-ID: Date: Tue, 7 Mar 2017 14:39:28 -0500 MIME-Version: 1.0 In-Reply-To: <87efy9ijrf.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Nir Soffer , qemu-devel@nongnu.org On 03/07/2017 03:16 AM, Markus Armbruster wrote: > John Snow writes: > >> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>> Nir Soffer writes: >>> >>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow wrote: >>>>> >>>>> >>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>> On Fri, Mar 3, 2017 at 8:54 PM, John Snow 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 >>>>>>> --- >>>>>>> >>>>>>> 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 >>>>>>