[libvirt] [PATCH v2] virsh: Allow other escape characters for console

[Eric Blake]

On 11/24/2011 04:03 AM, Michal Privoznik wrote:
> Currently virsh supports only ^] as escape character for console.
> However, some users might want to use something else. This patch
> creates such ability by specifying '-e' switch on virsh command
> line.
> ---
> diff to v1:
> -Eric's review included
> 
>  tools/console.c |   26 +++++++++++++++++++++-----
>  tools/console.h |    4 +++-
>  tools/virsh.c   |   40 +++++++++++++++++++++++++++++++---------
>  tools/virsh.pod |    5 +++++
>  4 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/console.c b/tools/console.c
> index 0f85bc7..060629f 100644
> --- a/tools/console.c
> +++ b/tools/console.c
> @@ -43,9 +43,11 @@
>  # include "memory.h"
>  # include "virterror_internal.h"
>  
> -
> -/* ie  Ctrl-]  as per telnet */
> -# define CTRL_CLOSE_BRACKET '\35'
> +/*
> + * Convert given character to control character.
> + * Basically, we take lower 6 bits.

Maybe tweak the comment with:

s/we take/we assume ASCII, and take/

> @@ -250,6 +253,7 @@ typedef struct __vshControl {
>                                     virDomainGetState is not supported */
>      bool useSnapshotOld;        /* cannot use virDomainSnapshotGetParent or
>                                     virDomainSnapshotNumChildren */
> +    const char *escapeChar;     /* Escape character for domain console */

This confused me in v1.  Here, you are storing the string representation
of the escape char, not the escape char itself.  How about tweaking the
comment:

/* String representation of console escape character */

> @@ -17095,15 +17100,17 @@ vshUsage(void)
>      fprintf(stdout, _("\n%s [options]... [<command_string>]"
>                        "\n%s [options]... <command> [args...]\n\n"
>                        "  options:\n"
> -                      "    -c | --connect <uri>    hypervisor connection URI\n"
> +                      "    -c | --connect=URI      hypervisor connection URI\n"
>                        "    -r | --readonly         connect readonly\n"
> -                      "    -d | --debug <num>      debug level [0-4]\n"
> +                      "    -d | --debug=num        debug level [0-4]\n"

Here, I'd write --debug=NUM, to make it obvious that NUM is a metasyntax
to be replaced by a number.

> @@ -17305,6 +17313,18 @@ vshParseArgv(vshControl *ctl, int argc, char **argv)
>          case 'l':
>              ctl->logfile = vshStrdup(ctl, optarg);
>              break;
> +        case 'e':
> +            len = strlen(optarg);
> +
> +            if ((len == 2 && *optarg == '^') ||
> +                (len == 1 && *optarg != '^')) {
> +                ctl->escapeChar = vshStrdup(ctl, optarg);

Mem leak - this overwrites malloc'd memory...

> +            } else {
> +                vshError(ctl, _("Invalid string '%s' for escape sequence"),
> +                         optarg);
> +                exit(EXIT_FAILURE);
> +            }
> +            break;
>          default:
>              vshError(ctl, _("unsupported option '-%c'. See --help."), arg);
>              exit(EXIT_FAILURE);
> @@ -17346,6 +17366,8 @@ main(int argc, char **argv)
>      ctl->imode = true;          /* default is interactive mode */
>      ctl->log_fd = -1;           /* Initialize log file descriptor */
>      ctl->debug = VSH_DEBUG_DEFAULT;
> +    ctl->escapeChar = vshStrdup(ctl, CTRL_CLOSE_BRACKET);

from here.  But why do you need vshStrdup in the first place?  Why not
just directly assign ctl->escapeChar = CTRL_CLOSE_BRACKET here, and
ctl->escapeChar = optarg above?

> +
>  
>      if (!setlocale(LC_ALL, "")) {
>          perror("setlocale");
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index db872dd..08b761d 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -92,6 +92,11 @@ option of the B<connect> command.
>  
>  Output elapsed time information for each command.
>  
> +=item B<-e>, B<--escape> I<string>
> +
> +Set alternative escape sequence for I<console> command. By default,
> +telnet's ^] is used.

It might look better in the man page to use B<^]> for emphasis.

ACK with nits fixed, and apologies for the slow review.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111130/06c7707c/attachment-0001.sig>