[libvirt] [PATCH] virsh: Allow other escape characters for console
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. What's your opinion? tools/console.c | 28 +++++++++++++++++++++++----- tools/console.h | 4 +++- tools/virsh.c | 20 ++++++++++++++++---- tools/virsh.pod | 5 +++++ 4 files changed, 47 insertions(+), 10 deletions(-) diff --git a/tools/console.c b/tools/console.c index 0f85bc7..db2e17e 100644 --- a/tools/console.c +++ b/tools/console.c @@ -43,9 +43,13 @@ # 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 5 bits unless given + * character is DEL (represented by '?'). Then + * we return 127 + */ +# define CONTROL(c) ((c) == '?' ? ((c) | 0x40) : ((c) & 0x1f)) # define VIR_FROM_THIS VIR_FROM_NONE @@ -66,6 +70,8 @@ struct virConsole { struct virConsoleBuffer streamToTerminal; struct virConsoleBuffer terminalToStream; + + char escapeChar; }; static int got_signal = 0; @@ -215,7 +221,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, virConsoleShutdown(con); return; } - if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) { virConsoleShutdown(con); return; } @@ -278,7 +284,18 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, } -int vshRunConsole(virDomainPtr dom, const char *dev_name) +static char +vshGetEscapeChar(const char *s) +{ + if (*s == '^') + return CONTROL(s[1]); + + return *s; +} + +int vshRunConsole(virDomainPtr dom, + const char *dev_name, + const char *escape_seq) { int ret = -1; struct termios ttyattr, rawattr; @@ -326,6 +343,7 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) goto cleanup; } + con->escapeChar = vshGetEscapeChar(escape_seq); con->st = virStreamNew(virDomainGetConnect(dom), VIR_STREAM_NONBLOCK); if (!con->st) diff --git a/tools/console.h b/tools/console.h index 9b05ff1..8cca08f 100644 --- a/tools/console.h +++ b/tools/console.h @@ -25,7 +25,9 @@ # ifndef WIN32 -int vshRunConsole(virDomainPtr dom, const char *dev_name); +int vshRunConsole(virDomainPtr dom, + const char *dev_name, + const char *escape_seq); # endif /* !WIN32 */ diff --git a/tools/virsh.c b/tools/virsh.c index 89fb4e7..1b84980 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -75,6 +75,9 @@ static char *progname; ((((int) ((T)->tv_sec - (U)->tv_sec)) * 1000000.0 + \ ((int) ((T)->tv_usec - (U)->tv_usec))) / 1000.0) +/* 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); + if (vshRunConsole(dom, name, ctl->escapeChar) == 0) ret = true; cleanup: @@ -16817,6 +16821,7 @@ vshDeinit(vshControl *ctl) vshReadlineDeinit(ctl); vshCloseLogFile(ctl); VIR_FREE(ctl->name); + VIR_FREE(ctl->escapeChar); if (ctl->conn) { int ret; if ((ret = virConnectClose(ctl->conn)) != 0) { @@ -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" " commands (non interactive mode):\n\n"), progname, progname); for (grp = cmdGroups; grp->name; grp++) { @@ -17009,13 +17015,14 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) {"connect", required_argument, NULL, 'c'}, {"readonly", no_argument, NULL, 'r'}, {"log", required_argument, NULL, 'l'}, + {"escape", required_argument, NULL, 'e'}, {NULL, 0, NULL, 0} }; /* Standard (non-command) options. The leading + ensures that no * argument reordering takes place, so that command options are * not confused with top-level virsh options. */ - while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:", opt, NULL)) != -1) { + while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:e:", opt, NULL)) != -1) { switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { @@ -17050,6 +17057,9 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) case 'l': ctl->logfile = vshStrdup(ctl, optarg); break; + case 'e': + ctl->escapeChar = vshStrdup(ctl, optarg); + break; default: vshError(ctl, _("unsupported option '-%c'. See --help."), arg); exit(EXIT_FAILURE); @@ -17091,6 +17101,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); + 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. + =back =head1 NOTES -- 1.7.3.4
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
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. + */ +# define CONTROL(c) ((c) ^ 0x40) # define VIR_FROM_THIS VIR_FROM_NONE @@ -66,6 +68,8 @@ struct virConsole { struct virConsoleBuffer streamToTerminal; struct virConsoleBuffer terminalToStream; + + char escapeChar; }; static int got_signal = 0; @@ -215,7 +219,7 @@ virConsoleEventOnStdin(int watch ATTRIBUTE_UNUSED, virConsoleShutdown(con); return; } - if (con->terminalToStream.data[con->terminalToStream.offset] == CTRL_CLOSE_BRACKET) { + if (con->terminalToStream.data[con->terminalToStream.offset] == con->escapeChar) { virConsoleShutdown(con); return; } @@ -278,7 +282,18 @@ virConsoleEventOnStdout(int watch ATTRIBUTE_UNUSED, } -int vshRunConsole(virDomainPtr dom, const char *dev_name) +static char +vshGetEscapeChar(const char *s) +{ + if (*s == '^') + return CONTROL(s[1]); + + return *s; +} + +int vshRunConsole(virDomainPtr dom, + const char *dev_name, + const char *escape_seq) { int ret = -1; struct termios ttyattr, rawattr; @@ -326,6 +341,7 @@ int vshRunConsole(virDomainPtr dom, const char *dev_name) goto cleanup; } + con->escapeChar = vshGetEscapeChar(escape_seq); con->st = virStreamNew(virDomainGetConnect(dom), VIR_STREAM_NONBLOCK); if (!con->st) diff --git a/tools/console.h b/tools/console.h index 9b05ff1..8cca08f 100644 --- a/tools/console.h +++ b/tools/console.h @@ -25,7 +25,9 @@ # ifndef WIN32 -int vshRunConsole(virDomainPtr dom, const char *dev_name); +int vshRunConsole(virDomainPtr dom, + const char *dev_name, + const char *escape_seq); # endif /* !WIN32 */ diff --git a/tools/virsh.c b/tools/virsh.c index 50fc98d..28e8d45 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -76,6 +76,9 @@ static char *progname; ((((int) ((T)->tv_sec - (U)->tv_sec)) * 1000000.0 + \ ((int) ((T)->tv_usec - (U)->tv_usec))) / 1000.0) +/* Default escape char Ctrl-] as per telnet */ +#define CTRL_CLOSE_BRACKET "^]" + /** * The log configuration */ @@ -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 */ } __vshControl; typedef struct vshCmdGrp { @@ -797,8 +801,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); + if (vshRunConsole(dom, name, ctl->escapeChar) == 0) ret = true; cleanup: @@ -17072,6 +17076,7 @@ vshDeinit(vshControl *ctl) vshReadlineDeinit(ctl); vshCloseLogFile(ctl); VIR_FREE(ctl->name); + VIR_FREE(ctl->escapeChar); if (ctl->conn) { int ret; if ((ret = virConnectClose(ctl->conn)) != 0) { @@ -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" " -h | --help this help\n" " -q | --quiet quiet mode\n" " -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" + " -l | --log=FILE output logging to file\n" + " -v short version\n" + " -V long version\n" + " --version[=TYPE] version, TYPE is short or long (default short)\n" + " -e | --escape <char> set escape sequence for console\n\n" " commands (non interactive mode):\n\n"), progname, progname); for (grp = cmdGroups; grp->name; grp++) { @@ -17254,7 +17261,7 @@ static bool vshParseArgv(vshControl *ctl, int argc, char **argv) { bool help = false; - int arg; + int arg, len; struct option opt[] = { {"debug", required_argument, NULL, 'd'}, {"help", no_argument, NULL, 'h'}, @@ -17264,13 +17271,14 @@ vshParseArgv(vshControl *ctl, int argc, char **argv) {"connect", required_argument, NULL, 'c'}, {"readonly", no_argument, NULL, 'r'}, {"log", required_argument, NULL, 'l'}, + {"escape", required_argument, NULL, 'e'}, {NULL, 0, NULL, 0} }; /* Standard (non-command) options. The leading + ensures that no * argument reordering takes place, so that command options are * not confused with top-level virsh options. */ - while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:", opt, NULL)) != -1) { + while ((arg = getopt_long(argc, argv, "+d:hqtc:vVrl:e:", opt, NULL)) != -1) { switch (arg) { case 'd': if (virStrToLong_i(optarg, NULL, 10, &ctl->debug) < 0) { @@ -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); + } 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); + 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. + =back =head1 NOTES -- 1.7.3.4
On 24.11.2011 12:03, 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(-)
Ping?
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@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 30.11.2011 20:29, Eric Blake wrote:
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(-)
ACK with nits fixed, and apologies for the slow review.
Fixed and pushed. Thanks. Michal
participants (2)
-
Eric Blake -
Michal Privoznik