Re: [libvirt] [PATCH v2] virsh: Allow other escape characters for console
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)