Re: [PATCH] Extending XL console command to take -e option that allows user to change escape character. Previously, Escape character was hardcoded as 0x1d i.e '^]'. Now, User has can provide escape character of his/her choice. It can be specified as any char starting with ^. For example, xl console -e ^a testDomain

[Doug Goldstein <cardoe@cardoe.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 10772 bytes --]

On 3/29/16 2:37 PM, sabiya wrote:

Sabiya,

Please have a look at:
http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

And review the sections titled "Signing off a patch" and "Write a good
description for each patch". Basically you need to provide a short
summary and then a newline and the rest of your commit message. You also
need to provide a signed off by line.


> ---
>  tools/console/client/main.c | 26 ++++++++++++++++++++++++--
>  tools/libxl/libxl.c         | 15 ++++++++++-----
>  tools/libxl/libxl.h         | 23 ++++++++++++++++++++---
>  tools/libxl/xl_cmdimpl.c    | 30 ++++++++++++++++++++++++------
>  tools/libxl/xl_cmdtable.c   |  5 +++--
>  5 files changed, 81 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/console/client/main.c b/tools/console/client/main.c
> index d006fdc..e175bbe 100644
> --- a/tools/console/client/main.c
> +++ b/tools/console/client/main.c
> @@ -35,6 +35,7 @@
>  #include <sys/select.h>
>  #include <err.h>
>  #include <string.h>
> +#include <ctype.h>
>  #ifdef __sun__
>  #include <sys/stropts.h>
>  #endif
> @@ -45,10 +46,12 @@
>  
>  #define ESCAPE_CHARACTER 0x1d
>  
> +
> +

extra white space change but that can be easily remedied.

>  static volatile sig_atomic_t received_signal = 0;
>  static char lockfile[sizeof (XEN_LOCK_DIR "/xenconsole.") + 8] = { 0 };
>  static int lockfd = -1;
> -
> +static char escapechar = ESCAPE_CHARACTER;
>  static void sighandler(int signum)
>  {
>  	received_signal = 1;
> @@ -214,7 +217,7 @@ static int console_loop(int fd, struct xs_handle *xs, char *pty_path,
>  			char msg[60];
>  
>  			len = read(STDIN_FILENO, msg, sizeof(msg));
> -			if (len == 1 && msg[0] == ESCAPE_CHARACTER) {
> +			if (len == 1 && msg[0] == escapechar) {
>  				return 0;
>  			} 
>  
> @@ -318,6 +321,19 @@ static void console_unlock(void)
>  	}
>  }
>  
> +
> +char getControlChar(char c) {
> +	return (c ^ 0x40);
> +}
> +
> +char getEscapeChar(const char *s)
> +{
> +    if (*s == '^')
> +        return getControlChar(s[1]);

You didn't follow my suggestions from the previous submission. This
unfortunately can crash and its not how we'd check for a character
supplied. I'd look at the last review I made. I know I suggested libvirt
but they internally store the escape character as a string "^]" but we
store it as a char 0x1d. So this can't be a copy and paste of libvirt.
What you've got is close.

> +
> +    return *s;
> +}
> +
>  int main(int argc, char **argv)
>  {
>  	struct termios attr;
> @@ -329,6 +345,7 @@ int main(int argc, char **argv)
>  	struct option lopt[] = {
>  		{ "type",     1, 0, 't' },
>  		{ "num",     1, 0, 'n' },
> +		{ "e",     1, 0, 'n' },

"escape" 1, 0, 'e'

>  		{ "help",    0, 0, 'h' },
>  		{ 0 },
>  
> @@ -363,6 +380,11 @@ int main(int argc, char **argv)
>  				exit(EINVAL);
>  			}
>  			break;
> +		case 'e' :
> +		    escapechar = getEscapeChar(optarg);
> +            break;
> +
> +
>  		default:
>  			fprintf(stderr, "Invalid argument\n");
>  			fprintf(stderr, "Try `%s --help' for more information.\n", 

You can drop below this point. Wei mentioned this unfortunately changes
stable API and there will be a little bit more work to make this part
land and we won't expect you to navigate creating a stable API as part
of this effort.


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 4cdc169..2b9ae22 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1715,14 +1715,16 @@ static void domain_destroy_domid_cb(libxl__egc *egc,
>  }
>  
>  int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
> -                       libxl_console_type type)
> +                       libxl_console_type type, char escapechar)
>  {
> +
> +    
>      GC_INIT(ctx);
>      char *p = GCSPRINTF("%s/xenconsole", libxl__private_bindir_path());
>      char *domid_s = GCSPRINTF("%d", domid);
>      char *cons_num_s = GCSPRINTF("%d", cons_num);
>      char *cons_type_s;
> -
> +    char *cons_escape_char = GCSPRINTF("%c", escapechar); 
>      switch (type) {
>      case LIBXL_CONSOLE_TYPE_PV:
>          cons_type_s = "pv";
> @@ -1734,7 +1736,10 @@ int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
>          goto out;
>      }
>  
> -    execl(p, p, domid_s, "--num", cons_num_s, "--type", cons_type_s, (void *)NULL);
> +   if(cons_escape_char == 0)
> +        execl(p, p, domid_s, "--num", cons_num_s, "--type", cons_type_s,(void *)NULL);
> +   else
> +        execl(p, p, domid_s, "--num", cons_num_s, "--type", cons_type_s, "--e", cons_escape_char, (void *)NULL);
>  
>  out:
>      GC_FREE;
> @@ -1823,7 +1828,7 @@ out:
>      return rc;
>  }
>  
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm, char escapechar)
>  {
>      uint32_t domid;
>      int cons_num;
> @@ -1832,7 +1837,7 @@ int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm)
>  
>      rc = libxl__primary_console_find(ctx, domid_vm, &domid, &cons_num, &type);
>      if ( rc ) return rc;
> -    return libxl_console_exec(ctx, domid, cons_num, type);
> +    return libxl_console_exec(ctx, domid, cons_num, type, escapechar);
>  }
>  
>  int libxl_primary_console_get_tty(libxl_ctx *ctx, uint32_t domid_vm,
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index f9e3ef5..4ac8cfd 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -218,6 +218,12 @@
>  #define LIBXL_HAVE_SOFT_RESET 1
>  
>  /*
> + if user does not specify any escape character sequence then 
> + Default escape character will be ^] 
> + */
> +
> +#define CTRL_CLOSE_BRACKET '\e'



> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> @@ -1317,15 +1323,26 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_k
>  int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs);
>  
>  int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
> -int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type);
> +
> +
> +/*
> + *  Escapechar can be specified , If specified given escape char will
> + *  be used for terminating logging. Otherwise default Ctrl-] will be used,
> + */
> +int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type, char escapechar);
>  /* libxl_primary_console_exec finds the domid and console number
>   * corresponding to the primary console of the given vm, then calls
>   * libxl_console_exec with the right arguments (domid might be different
>   * if the guest is using stubdoms).
>   * This function can be called after creating the device model, in
>   * case of HVM guests, and before libxl_run_bootloader in case of PV
> - * guests using pygrub. */
> -int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm);
> + * guests using pygrub. 
> + *
> + * Escapechar can be specified , If specified given escape char will
> + * be used for terminating logging. Otherwise default Ctrl-] will be used,
> + */
> + 
> +int libxl_primary_console_exec(libxl_ctx *ctx, uint32_t domid_vm, char escapechar);
>  
>  /* libxl_console_get_tty retrieves the specified domain's console tty path
>   * and stores it in path. Caller is responsible for freeing the memory.
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 990d3c9..da290b6 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1,3 +1,4 @@
> +
>  /*
>   * Copyright (C) 2009      Citrix Ltd.
>   * Author Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> @@ -41,7 +42,9 @@
>  #include "libxl_json.h"
>  #include "libxlutil.h"
>  #include "xl.h"
> -
> +#define ESCAPE_CHARACTER 0x1d
> +char getControlChar(char c);
> +char getEscapeChar(const char *s);
>  /* For calls which return an errno on failure */
>  #define CHK_ERRNOVAL( call ) ({                                         \
>          int chk_errnoval = (call);                                      \
> @@ -2623,7 +2626,7 @@ static void autoconnect_console(libxl_ctx *ctx_ignored,
>      postfork();
>  
>      sleep(1);
> -    libxl_primary_console_exec(ctx, bldomid);
> +    libxl_primary_console_exec(ctx, bldomid, ESCAPE_CHARACTER);
>      /* Do not return. xl continued in child process */
>      perror("xl: unable to exec console client");
>      _exit(1);
> @@ -3411,14 +3414,24 @@ int main_cd_insert(int argc, char **argv)
>      cd_insert(domid, virtdev, file);
>      return 0;
>  }
> +char getControlChar(char c) {
> +    return c ^ 0x40;
> +}
> +char getEscapeChar(const char *s)
> +{
> +    if (*s == '^')
> +        return getControlChar(s[1]);
>  
> +    return *s;
> +}
>  int main_console(int argc, char **argv)
>  {
>      uint32_t domid;
>      int opt = 0, num = 0;
> +    char escapechar = ESCAPE_CHARACTER;
>      libxl_console_type type = 0;
>  
> -    SWITCH_FOREACH_OPT(opt, "n:t:", NULL, "console", 1) {
> +    SWITCH_FOREACH_OPT(opt, "n:t:e", NULL, "console", 1) {
>      case 't':
>          if (!strcmp(optarg, "pv"))
>              type = LIBXL_CONSOLE_TYPE_PV;
> @@ -3432,13 +3445,18 @@ int main_console(int argc, char **argv)
>      case 'n':
>          num = atoi(optarg);
>          break;
> +    case 'e':
> +        escapechar = getEscapeChar(optarg);
> +        break;
>      }
>  
>      domid = find_domain(argv[optind]);
>      if (!type)
> -        libxl_primary_console_exec(ctx, domid);
> -    else
> -        libxl_console_exec(ctx, domid, num, type);
> +        libxl_primary_console_exec(ctx, domid, escapechar);
> +    else 
> +        libxl_console_exec(ctx, domid, num, type, escapechar); 
> +
> +        
>      fprintf(stderr, "Unable to attach console\n");
>      return 1;
>  }
> diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
> index fdc1ac6..794c7c1 100644
> --- a/tools/libxl/xl_cmdtable.c
> +++ b/tools/libxl/xl_cmdtable.c
> @@ -133,8 +133,9 @@ struct cmd_spec cmd_table[] = {
>        &main_console, 0, 0,
>        "Attach to domain's console",
>        "[options] <Domain>\n"
> -      "-t <type>       console type, pv or serial\n"
> -      "-n <number>     console number"
> +      "-t <type>             console type, pv or serial\n"
> +      "-n <number>           console number\n"
> +      "-e <escapechar>       escape character"
>      },
>      { "vncviewer",
>        &main_vncviewer, 0, 0,
> 


-- 
Doug Goldstein


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel