Re: [libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode

On 28/09/16 21:27, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1379895 > > Introduced by commit id '834c5720'. > > During the code motion and creation of vsh.c, the function 'vshDeinit()' > in the (new) vsh.c was altered from whence it came in virsh.c such that > calling 'vshReadlineDeinit(ctl)' was conditional on "ctl->imode". > > This causes a problem for the interactive running if the "quit" and "exit" > commands are used because 'cmdQuit' will clear ctl->imode, thus when the > interactive loop in main() of virsh.c exits because ctl->mode is clear and > virshDeinit is called which calls vshDeinit, the history file is now not > written. Conversely, if one had exited the interactive loop via pressing > <ctrl>D the file would be created because loop control is broken on EOF > and ctl->imode is not set to false. > > This patch will remove the conditional call to vshReadlineDeinit and > restore the former behaviour. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > I realize some people don't like the comment I added; however, I'd > rather be "safe" when purely reading the code than expect someone > to do a git log -p looking at commit messages for "removed" lines of code. > > An alternative approach would be to create/set a new boolean value > such as "iquit" (or "iexit") during cmdQuit and then in vshDeinit make > calling vshReadlineDeinit() conditional on "ctl->imode || ctl->iquit" > > Calling the vshReadlineDeinit during non interactive mode would have no > affect since vshReadlineInit is only called if ctl->imode is set. The > Deinit function will essentially do nothing other than a couple of > VIR_FREE(NULL); and one extra "if" > > tools/vsh.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/tools/vsh.c b/tools/vsh.c > index 041113f..eba3f0f 100644 > --- a/tools/vsh.c > +++ b/tools/vsh.c > @@ -3093,8 +3093,12 @@ vshInitReload(vshControl *ctl) > void > vshDeinit(vshControl *ctl) > { > - if (ctl->imode) > - vshReadlineDeinit(ctl); > + /* Don't make calling vshReadlineDeinit conditional on imode. During > + * interactive mode when "quit" or "exit" is typed, 'imode' is set > + * to false so if this were conditional on imode, then history wouldn't > + * be written when the "quit" or "exit" commands were used instead of > + * when <ctrl>D is used */ Well, the commit message is already pretty verbose. I do understand your concern about having to wade through log -p output, especially when you encounter something like 834c5720 which truly is evil (I started over like 3 times with 3 different ugly results), but once you make friends with gitk, tig or whatever interactive tool, everything becomes a little more convenient. In conclusion, I think something like "Don't make calling of vshReadlineDeinit conditional on active interactive mode." would suffice. ACK Erik

> + vshReadlineDeinit(ctl); > vshCloseLogFile(ctl); > } > >