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

[Erik Skultety]

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




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [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
>> 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 
>> ---
>>  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 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);
>>  }
>>  
>>
> 
> 

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

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

[Erik Skultety]

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




signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[John Ferlan]

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
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 
---
 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 D is used */
+vshReadlineDeinit(ctl);
 vshCloseLogFile(ctl);
 }
 
-- 
2.7.4

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