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@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 >> > > 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); >>> } >>> >>> >> >>
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list