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@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