[libvirt] [PATCH] vsh: Write out history on "quit" or "exit" in interactive mode
Erik Skultety
eskultet at redhat.com
Thu Sep 29 10:58:03 UTC 2016
On 29/09/16 12:49, John Ferlan wrote:
>
>
> 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
Hmm, in my opinion that's just telling us that either noone is truly
depending on that feature or they're just used to hit <ctrl>D
automatically...
> 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?
>
Sure, I'm sorry I didn't write it explicitly, I automatically assumed
that to be safe, since a) it's a bugfix, b) it's a regression that
should really be fixed as soon as possible, my bad :)...
Erik
> John
>>> + vshReadlineDeinit(ctl);
>>> vshCloseLogFile(ctl);
>>> }
>>>
>>>
>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160929/81a2decf/attachment-0001.sig>
More information about the libvir-list
mailing list