tools: explain that '^' means 'Ctrl' for console escape sequence

[libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Daniel P. Berrangé 4 years, 1 month ago

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tools/virsh-domain.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index 8591e483a5..ada0189685 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -3002,7 +3002,11 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
     }
 
     vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom));
-    vshPrintExtra(ctl, _("Escape character is %s\n"), priv->escapeChar);
+    vshPrintExtra(ctl, _("Escape character is %s"), priv->escapeChar);
+    if (priv->escapeChar[0] == '^') {
+        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
+    }
+    vshPrintExtra(ctl, "\n");
     fflush(stdout);
     if (virshRunConsole(ctl, dom, name, flags) == 0)
         return true;
-- 
2.24.1

Re: [libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Michal Prívozník 4 years, 1 month ago

On 27. 3. 2020 16:11, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  tools/virsh-domain.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 8591e483a5..ada0189685 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -3002,7 +3002,11 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
>      }
>  
>      vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom));
> -    vshPrintExtra(ctl, _("Escape character is %s\n"), priv->escapeChar);
> +    vshPrintExtra(ctl, _("Escape character is %s"), priv->escapeChar);
> +    if (priv->escapeChar[0] == '^') {
> +        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> +    }
> +    vshPrintExtra(ctl, "\n");
>      fflush(stdout);
>      if (virshRunConsole(ctl, dom, name, flags) == 0)
>          return true;
> 


Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

Re: [libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Daniel P. Berrangé 4 years, 1 month ago

On Fri, Mar 27, 2020 at 04:34:48PM +0100, Michal Prívozník wrote:
> On 27. 3. 2020 16:11, Daniel P. Berrangé wrote:
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  tools/virsh-domain.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 8591e483a5..ada0189685 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -3002,7 +3002,11 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
> >      }
> >  
> >      vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom));
> > -    vshPrintExtra(ctl, _("Escape character is %s\n"), priv->escapeChar);
> > +    vshPrintExtra(ctl, _("Escape character is %s"), priv->escapeChar);
> > +    if (priv->escapeChar[0] == '^') {
> > +        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> > +    }

I'll remove the {} to keep syntax-check happy about single line "if"

> > +    vshPrintExtra(ctl, "\n");
> >      fflush(stdout);
> >      if (virshRunConsole(ctl, dom, name, flags) == 0)
> >          return true;
> > 
> 
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Michal

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Michal Prívozník 4 years, 1 month ago

On 27. 3. 2020 16:42, Daniel P. Berrangé wrote:
> On Fri, Mar 27, 2020 at 04:34:48PM +0100, Michal Prívozník wrote:
>> On 27. 3. 2020 16:11, Daniel P. Berrangé wrote:
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>  tools/virsh-domain.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
>>> index 8591e483a5..ada0189685 100644
>>> --- a/tools/virsh-domain.c
>>> +++ b/tools/virsh-domain.c
>>> @@ -3002,7 +3002,11 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom,
>>>      }
>>>  
>>>      vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom));
>>> -    vshPrintExtra(ctl, _("Escape character is %s\n"), priv->escapeChar);
>>> +    vshPrintExtra(ctl, _("Escape character is %s"), priv->escapeChar);
>>> +    if (priv->escapeChar[0] == '^') {
>>> +        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
>>> +    }
> 
> I'll remove the {} to keep syntax-check happy about single line "if"


Speaking of which. That exception, while it might have served the
purpose in the past, now that the code base has grown in size I think
the less exceptions to formatting style we have the better. So maybe we
can start thinking about requiring braces for every if() ? Just a friday
evening thought.

Michal

Re: [libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Andrea Bolognani 4 years, 1 month ago

On Fri, 2020-03-27 at 16:52 +0100, Michal Prívozník wrote:
> On 27. 3. 2020 16:42, Daniel P. Berrangé wrote:
> > > > +    if (priv->escapeChar[0] == '^') {
> > > > +        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> > > > +    }
> > 
> > I'll remove the {} to keep syntax-check happy about single line "if"
> 
> Speaking of which. That exception, while it might have served the
> purpose in the past, now that the code base has grown in size I think
> the less exceptions to formatting style we have the better. So maybe we
> can start thinking about requiring braces for every if() ? Just a friday
> evening thought.

I'm all for it.

We should really just figure out a clang-format configuration that
approximates reasonably well our current code style, throw out
every exception that can't be represented, and just tell people to
run the tool before submitting code.

Incidentally, that's the approach both Rust and Go have followed
since the very beginning.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] tools: explain that '^' means 'Ctrl' for console escape sequence

Posted by Daniel P. Berrangé 4 years ago

On Fri, Mar 27, 2020 at 08:40:31PM +0100, Andrea Bolognani wrote:
> On Fri, 2020-03-27 at 16:52 +0100, Michal Prívozník wrote:
> > On 27. 3. 2020 16:42, Daniel P. Berrangé wrote:
> > > > > +    if (priv->escapeChar[0] == '^') {
> > > > > +        vshPrintExtra(ctl, " (Ctrl + %c)", priv->escapeChar[1]);
> > > > > +    }
> > > 
> > > I'll remove the {} to keep syntax-check happy about single line "if"
> > 
> > Speaking of which. That exception, while it might have served the
> > purpose in the past, now that the code base has grown in size I think
> > the less exceptions to formatting style we have the better. So maybe we
> > can start thinking about requiring braces for every if() ? Just a friday
> > evening thought.
> 
> I'm all for it.

Braces (or not) around single line ifs is not something I'd suggest we
spend any time changing in isolation as it is disruptive for not clear
win.
 
> We should really just figure out a clang-format configuration that
> approximates reasonably well our current code style, throw out
> every exception that can't be represented, and just tell people to
> run the tool before submitting code.
> 
> Incidentally, that's the approach both Rust and Go have followed
> since the very beginning.

Yeah, I think it is really a good approach, as it removes any kind of
ambiguity. A global switch to clang-format is the time at which we could
consider changing single line if brace policy.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|