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