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

[Eric Blake]

On 11/22/2011 09:18 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.
> ---
> Okay, this patch is meant as RFC mainly but if it got enough ACKs
> I will not hesitate to push it. My biggest concern is the way
> of telling virsh customized sequence. I am not big fan of new switch,
> however we lack virsh.conf in $conf_dir. Maybe it is the right time
> for creating it.
> Another approach is to pass the sequence as parameter directly to
> 'console' command.

I'm leaning 60-40 towards 'virsh -e', since both 'virsh console' and
'virsh start --console' would benefit from shared code, rather than
having to make both of those functions parse in an alternate escape
character.

I agree that a virsh.conf would make it nice to set preferences without
having to always spell it out on the command line, in which case having
virsh.conf serve as an alternate to 'virsh -e ... console' makes more
sense than having it serve as an alternate to 'virsh console -e ...'
(that is, it seems like having virsh.conf override defaults for global
settings makes more sense than having it override defaults of
per-command settings), so that would sway my opinion to 70-30 in favor
of a global -e.

> -
> -/* ie  Ctrl-]  as per telnet */
> -# define CTRL_CLOSE_BRACKET '\35'
> +/*
> + * Convert given character to control character.
> + * Basically, we take lower 5 bits unless given
> + * character is DEL (represented by '?'). Then
> + * we return 127
> + */
> +# define CONTROL(c) ((c) == '?' ? ((c) | 0x40) : ((c) & 0x1f))

I've usually seen this written:

CONTROL(c) ((c) ^ 0x40)

This is ASCII-specific, but do we care about EBCDIC?

>  
> -int vshRunConsole(virDomainPtr dom, const char *dev_name)
> +static char
> +vshGetEscapeChar(const char *s)
> +{
> +    if (*s == '^')
> +        return CONTROL(s[1]);
> +
> +    return *s;
> +}

Should we sanity check that the string s is exactly 1 or 2 bytes, that
it is not a 1-byte ^, and that if it is 2 bytes, it starts with ^?

>  
> +/* Default escape char Ctrl-] as per telnet */
> +#define CTRL_CLOSE_BRACKET "^]"
> +
>  /**
>   * The log configuration
>   */
> @@ -249,6 +252,7 @@ typedef struct __vshControl {
>                                     virDomainGetState is not supported */
>      bool useSnapshotOld;        /* cannot use virDomainSnapshotGetParent or
>                                     virDomainSnapshotNumChildren */
> +    const char *escapeChar;     /* Escape character for domain console */
>  } __vshControl;
>  
>  typedef struct vshCmdGrp {
> @@ -796,8 +800,8 @@ cmdRunConsole(vshControl *ctl, virDomainPtr dom, const char *name)
>      }
>  
>      vshPrintExtra(ctl, _("Connected to domain %s\n"), virDomainGetName(dom));
> -    vshPrintExtra(ctl, "%s", _("Escape character is ^]\n"));
> -    if (vshRunConsole(dom, name) == 0)
> +    vshPrintExtra(ctl, _("Escape character is %s\n"), ctl->escapeChar);

This won't work.  You want to undo the CONTROL() conversion, and output
a literal ^ followed by the counterpart character, as in:

vshPrintExtra(ctl, _("Escape character is ^%c\n"), CONTROL(ctl->escapeChar))

assuming that the control character is non-printable, and the
counterpart is printable.  Maybe you need a c_isprint() call in there?

> @@ -16848,7 +16853,8 @@ vshUsage(void)
>                        "    -t | --timing           print timing information\n"
>                        "    -l | --log <file>       output logging to file\n"
>                        "    -v | --version[=short]  program version\n"
> -                      "    -V | --version=long     version and full options\n\n"
> +                      "    -V | --version=long     version and full options\n"
> +                      "    -e | --escape <char>    set escape sequence for console\n\n"

Well, we aren't very consistent in that formatting.  I'd almost prefer
we change to a single format:

-l | --log=FILE     log to FILE
-v                  short version
-V                  long version
  --version[=TYPE]  version, TYPE is short or long (default short)
-e | --escape=CHAR  set console escape sequence to CHAR

-- 
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/20111122/3dcecb00/attachment-0001.sig>