Re: Interested to participate in Outreachy Program

[Doug Goldstein <cardoe@cardoe.com>]

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

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 <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
>  
> +# 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 <stefano.stabellini@eu.citrix.com>
> @@ -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] <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,

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


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