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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org