Message ID | 20170303185427.32681-1-jsnow@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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>
On 03/03/2017 02:26 PM, Nir Soffer wrote: > 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. > 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 <nirsof@gmail.com> >
On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: > > > On 03/03/2017 02:26 PM, Nir Soffer wrote: >> 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. >> > > 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 <nirsof@gmail.com> >>
Nir Soffer <nirsof@gmail.com> writes: > On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >> >> >> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>> 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') 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 <nirsof@gmail.com> >>>
On Mon, Mar 06, 2017 at 09:18:29AM +0100, Markus Armbruster wrote: > Nir Soffer <nirsof@gmail.com> writes: > > > On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@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 <nirsof@gmail.com> > >>> >
On 03/06/2017 03:18 AM, Markus Armbruster wrote: > Nir Soffer <nirsof@gmail.com> writes: > >> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>> >>> >>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>> 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') > > 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 <nirsof@gmail.com> >>>>
John Snow <jsnow@redhat.com> writes: > On 03/06/2017 03:18 AM, Markus Armbruster wrote: >> Nir Soffer <nirsof@gmail.com> writes: >> >>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>> >>>> >>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>> 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') >> >> 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 <nirsof@gmail.com> >>>>>
On 03/07/2017 03:16 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>> Nir Soffer <nirsof@gmail.com> writes: >>> >>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>>> >>>>> >>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>> 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') >>> >>> 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 think this is good enough and useful as is. >>>>>> >>>>>> Reviewed-by: Nir Soffer <nirsof@gmail.com> >>>>>>
On 03/07/2017 03:16 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>> Nir Soffer <nirsof@gmail.com> writes: >>> >>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>>> >>>>> >>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>> 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') >>> >>> 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 <nirsof@gmail.com> >>>>>>
John Snow <jsnow@redhat.com> writes: > On 03/07/2017 03:16 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>>> Nir Soffer <nirsof@gmail.com> writes: >>>> >>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>>>> >>>>>> >>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>>> 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') >>>> >>>> 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 login, I would've filed a bug already. I'm afraid all I can offer meanwhile is INPUTRC: Any user can customize programs that use Readline by putting commands in an "inputrc" file, conventionally in his home directory. The name of this file is taken from the value of the environment variable `INPUTRC'. If that variable is unset, the default is `~/.inputrc'. If that file does not exist or cannot be read, the ultimate default is `/etc/inputrc'. >>>>>>>> + 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> >>>>>>>
On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > >> On 03/07/2017 03:16 AM, Markus Armbruster wrote: >>> John Snow <jsnow@redhat.com> writes: >>> >>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>>>> Nir Soffer <nirsof@gmail.com> writes: >>>>> >>>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>>>>> >>>>>>> >>>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>>>> 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') >>>>> >>>>> 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 login, I would've > filed a bug already. > > I'm afraid all I can offer meanwhile is INPUTRC: > > Any user can customize programs that use Readline by putting > commands in an "inputrc" file, conventionally in his home directory. > The name of this file is taken from the value of the environment > variable `INPUTRC'. If that variable is unset, the default is > `~/.inputrc'. If that file does not exist or cannot be read, the > ultimate default is `/etc/inputrc'. > Having a way to limit history globally looks good enough. Note that python readline does not report the readline limit correctly (get_history_length() returns -1), but saving history uses the limit defined in .inputrc. Playing with this, I found a nice bug - if you set history size to N, and your history file contains 2 * N items or more, python segfaults when entering the first input line. I'll file a python bug later. >>>>>>>>> + 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> >>>>>>>>
Nir Soffer <nirsof@gmail.com> writes: > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com> wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 03/07/2017 03:16 AM, Markus Armbruster wrote: >>>> John Snow <jsnow@redhat.com> writes: >>>> >>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote: >>>>>> Nir Soffer <nirsof@gmail.com> writes: >>>>>> >>>>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: >>>>>>>>> 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') >>>>>> >>>>>> 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 login, I would've >> filed a bug already. >> >> I'm afraid all I can offer meanwhile is INPUTRC: >> >> Any user can customize programs that use Readline by putting >> commands in an "inputrc" file, conventionally in his home directory. >> The name of this file is taken from the value of the environment >> variable `INPUTRC'. If that variable is unset, the default is >> `~/.inputrc'. If that file does not exist or cannot be read, the >> ultimate default is `/etc/inputrc'. >> > > Having a way to limit history globally looks good enough. Certainly good enough for merging qmp-shell patches. > Note that python readline does not report the readline > limit correctly (get_history_length() returns -1), but saving > history uses the limit defined in .inputrc. > > Playing with this, I found a nice bug - if you set history > size to N, and your history file contains 2 * N items or more, > python segfaults when entering the first input line. > > I'll file a python bug later. Please do. If you could throw in a (separate) request for letting us set rl_readline_name, that would be great. [...]
On Mon, Mar 13, 2017 at 8:04 AM Markus Armbruster <armbru@redhat.com> wrote: > Nir Soffer <nirsof@gmail.com> writes: > > > On Wed, Mar 8, 2017 at 8:29 AM, Markus Armbruster <armbru@redhat.com> > wrote: > >> John Snow <jsnow@redhat.com> writes: > >> > >>> On 03/07/2017 03:16 AM, Markus Armbruster wrote: > >>>> John Snow <jsnow@redhat.com> writes: > >>>> > >>>>> On 03/06/2017 03:18 AM, Markus Armbruster wrote: > >>>>>> Nir Soffer <nirsof@gmail.com> writes: > >>>>>> > >>>>>>> On Fri, Mar 3, 2017 at 9:29 PM, John Snow <jsnow@redhat.com> > wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On 03/03/2017 02:26 PM, Nir Soffer wrote: > >>>>>>>>> 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') > >>>>>> > >>>>>> 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 login, I > would've > >> filed a bug already. > >> > >> I'm afraid all I can offer meanwhile is INPUTRC: > >> > >> Any user can customize programs that use Readline by putting > >> commands in an "inputrc" file, conventionally in his home directory. > >> The name of this file is taken from the value of the environment > >> variable `INPUTRC'. If that variable is unset, the default is > >> `~/.inputrc'. If that file does not exist or cannot be read, the > >> ultimate default is `/etc/inputrc'. > >> > > > > Having a way to limit history globally looks good enough. > > Certainly good enough for merging qmp-shell patches. > > > Note that python readline does not report the readline > > limit correctly (get_history_length() returns -1), but saving > > history uses the limit defined in .inputrc. > > > > Playing with this, I found a nice bug - if you set history > > size to N, and your history file contains 2 * N items or more, > > python segfaults when entering the first input line. > > > > I'll file a python bug later. > > Please do. > Reported here: http://bugs.python.org/issue29854 https://github.com/python/cpython/pull/728 If you could throw in a (separate) request for letting us set > rl_readline_name, that would be great. > Will do. > > [...] >
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) + 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:
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(+)