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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> ---
>  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);
>  }
>  
> 


Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]