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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list