Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
On Mon, Mar 13, 2017 at 8:04 AM Markus Armbruster wrote: > Nir Soffer writes: > > > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster > wrote: > >> John Snow writes: > >> > >>> 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
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
Nir Soffer writes: > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster wrote: >> John Snow writes: >> >>> 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
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster wrote: > John Snow writes: > >> 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
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
John Snow writes: > 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" ...) How sad. If https://bugs.python.org/ didn't require a
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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) >>> +ex
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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. > There's the winning ticket! >>> +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 th
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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. >> +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 >
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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...!) > > 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. > +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
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
On Mon, Mar 06, 2017 at 09:18:29AM +0100, Markus Armbruster wrote: > Nir Soffer writes: > > > On Fri, Mar 3, 2017 at 9:29 PM, John Snow 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 > >>> > -- /kashyap
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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)? 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. > 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? +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 >>>
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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') >>> >>> 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. 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 > >>> +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 >>
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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') >> >> 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. >> +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 >
Re: [Qemu-devel] [PATCH v2] qmp-shell: add persistent command history
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') > > 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