On 3/23/16 11:34 AM, sabiya kazi wrote: > Hi Doug, > Can you have a look at patch and let me know if everything > is correct, I think things are good. > > I would also like to have a word with you for deciding timeline for > project. Meantime, I have started reading stuff about rust language. > > > Regards, > -Sabiya > ​ > Inlining the patch since it was sent as an attachment.. > diff --git a/tools/Makefile b/tools/Makefile > index 3f45fb9..1c2fb79 100644 > --- a/tools/Makefile > +++ b/tools/Makefile > @@ -1,4 +1,4 @@ > -XEN_ROOT = $(CURDIR)/.. > + XEN_ROOT = $(CURDIR)/.. > include $(XEN_ROOT)/tools/Rules.mk drop this change > > SUBDIRS-y := > diff --git a/tools/console/client/main.c b/tools/console/client/main.c > index d006fdc..199432c 100644 > --- a/tools/console/client/main.c > +++ b/tools/console/client/main.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > #ifdef __sun__ > #include > #endif > @@ -45,10 +46,12 @@ > > #define ESCAPE_CHARACTER 0x1d > > +# define CONTROL(c) ((c) ^ 0x40) not really necessary as a define. this can just go into the function with a comment > + > 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,14 @@ static void console_unlock(void) > } > } > > +char getEscapeChar(const char *s) > +{ > + if (*s == '^') > + return CONTROL(toupper(s[1])); This has the possibility to crash. Not really sure why we would want this at all tbh. The valid range of escape characters should be "a-z A-Z @ [ \ ] ^ _" so really we should only ever deal with 1 character and that character needs to be in the range of: if ( s <= 'a' && s <= 'z' || s <= '@' && s <= '_' ) escapechar = s; else tell_the_user_they_did_wrong(); > + > + return *s; > +} > + > int main(int argc, char **argv) > { > struct termios attr; > @@ -329,6 +340,7 @@ int main(int argc, char **argv) > struct option lopt[] = { > { "type", 1, 0, 't' }, > { "num", 1, 0, 'n' }, > + { "escapechar", 1, 0, 'n' }, the last field should be 'e' > { "help", 0, 0, 'h' }, > { 0 }, > > @@ -363,6 +375,11 @@ int main(int argc, char **argv) > exit(EINVAL); > } > break; > + case 'e' : > + escapechar = getEscapeChar(optarg); > + break; white space is off here > + > + > default: > fprintf(stderr, "Invalid argument\n"); > fprintf(stderr, "Try `%s --help' for more information.\n", > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index 4cdc169..86ee670 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,13 +1736,17 @@ 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 == NULL) this won't ever be true because of the GCSPRINTF() above. I think you mean to check if escapechar == 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, "--escapechar", cons_escape_char, (void *)NULL); > > out: > GC_FREE; > return ERROR_FAIL; > } > > + unnecessary white space change > int libxl_console_get_tty(libxl_ctx *ctx, uint32_t domid, int cons_num, > libxl_console_type type, char **path) > { > @@ -1823,7 +1829,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 +1838,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..fa87f8d 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 > @@ -41,7 +42,8 @@ > #include "libxl_json.h" > #include "libxlutil.h" > #include "xl.h" > - > +# define CONTROL(c) ((c) ^ 0x40) > + char getEscapeChar(const char *s); Let's drop this given my above comments > /* For calls which return an errno on failure */ > #define CHK_ERRNOVAL( call ) ({ \ > int chk_errnoval = (call); \ > @@ -2623,7 +2625,7 @@ static void autoconnect_console(libxl_ctx *ctx_ignored, > postfork(); > > sleep(1); > - libxl_primary_console_exec(ctx, bldomid); > + libxl_primary_console_exec(ctx, bldomid, 0); We probably should just bring the ESCAPE_CHARACTER #define into libxl and just supply that here by default. > /* Do not return. xl continued in child process */ > perror("xl: unable to exec console client"); > _exit(1); > @@ -3411,14 +3413,21 @@ int main_cd_insert(int argc, char **argv) > cd_insert(domid, virtdev, file); > return 0; > } > +char getEscapeChar(const char *s) > +{ > + if (*s == '^') > + return CONTROL(toupper(s[1])); see above comments > > + return *s; > +} > int main_console(int argc, char **argv) > { > uint32_t domid; > int opt = 0, num = 0; > + char escapechar = 0; > 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 +3441,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] \n" > - "-t console type, pv or serial\n" > - "-n console number" > + "-t console type, pv or serial\n" > + "-n console number\n" > + "-e escape character" > }, > { "vncviewer", > &main_vncviewer, 0, 0, Overall this change makes me think about some rough edges of the libxl API but I think this is the minimum amount to get the feature working without worrying about API bits. -- Doug Goldstein