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

[John Ferlan]

On 09/29/2016 03:06 AM, Erik Skultety wrote:
> 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 at redhat.com>
>> ---
>>  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
> 

I'll adjust the message before pushing. And yes, I use gitk and git log
-p frequently, but not everyone does and the concern is someone alters
the code without looking at the history. Considering it took 13+ months
to have someone realize it - I wanted some way to make clear that
someone needs to look at the history before just changing this. I can
completely understand why the conditional was added though since the
Init calls gate on ctl->imode

Shall I assume it's "safe" for the freeze too?

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