Re: [PATCH 5 of 5 V2] tools/xl: Remus - Network buffering cmdline switch

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2013-08-29 at 15:16 -0700, Shriram Rajagopalan wrote:
> @@ -418,6 +417,13 @@ Generally useful for debugging.
>  
>  Disable memory checkpoint compression.
>  
> +=item B<-n>
> +
> +Enable network output buffering.  The default script used to configure
> +network buffering is /etc/xen/scripts/remus-netbuf-setup. If you wish to
> +use a custom script, set the global variable "remus.default.netbufscript"
> +in /etc/xen/xl.conf to point to your script.

Is this not the sort of thing you might want to control on a per domain
basis? Or perhaps on a per- "xl remus" granularity?

> +
>  =item B<-s> I<sshcommand>
>  
>  Use <sshcommand> instead of ssh.  String will be passed to sh.
> diff -r b8839e0b61c2 -r 66e6a3c64566 tools/libxl/xl.c
> --- a/tools/libxl/xl.c	Thu Aug 29 14:50:01 2013 -0700
> +++ b/tools/libxl/xl.c	Thu Aug 29 15:06:47 2013 -0700
> @@ -46,6 +46,7 @@ char *default_vifscript = NULL;
>  char *default_bridge = NULL;
>  char *default_gatewaydev = NULL;
>  char *default_vifbackend = NULL;
> +char *default_remus_netbufscript = NULL;
>  enum output_format default_output_format = OUTPUT_FORMAT_JSON;
>  int claim_mode = 1;
>  
> @@ -177,6 +178,11 @@ static void parse_global_config(const ch
>      if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
>          claim_mode = l;
>  
> +    if (!xlu_cfg_get_string (config, "remus.default.netbufscript", &buf, 0)) {
> +        free(default_remus_netbufscript);
> +        default_remus_netbufscript = strdup(buf);
> +    }

xlu_cfg_replace_string? I think the patterns you have copied predate
this function and/or were more copying which noone noticed.

(If you were feeling exceptionally keen you could update them all, not a
requirement though)

> +
>      xlu_cfg_destroy(config);
>  }
>  
> diff -r b8839e0b61c2 -r 66e6a3c64566 tools/libxl/xl.h
> --- a/tools/libxl/xl.h	Thu Aug 29 14:50:01 2013 -0700
> +++ b/tools/libxl/xl.h	Thu Aug 29 15:06:47 2013 -0700
> @@ -152,6 +152,7 @@ extern char *default_vifscript;
>  extern char *default_bridge;
>  extern char *default_gatewaydev;
>  extern char *default_vifbackend;
> +extern char *default_remus_netbufscript;
>  extern char *blkdev_start;
>  
>  enum output_format {
> diff -r b8839e0b61c2 -r 66e6a3c64566 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Aug 29 14:50:01 2013 -0700
> +++ b/tools/libxl/xl_cmdimpl.c	Thu Aug 29 15:06:47 2013 -0700
> @@ -7070,8 +7070,9 @@ int main_remus(int argc, char **argv)
>      r_info.interval = 200;
>      r_info.blackhole = 0;
>      r_info.compression = 1;
> -
> -    SWITCH_FOREACH_OPT(opt, "bui:s:e", NULL, "remus", 2) {
> +    r_info.netbuf = 0;
> +
> +    SWITCH_FOREACH_OPT(opt, "buni:s:e", NULL, "remus", 2) {
>      case 'i':
>          r_info.interval = atoi(optarg);
>          break;
> @@ -7081,6 +7082,9 @@ int main_remus(int argc, char **argv)
>      case 'u':
>          r_info.compression = 0;
>          break;
> +    case 'n':
> +        r_info.netbuf = 1;
> +        break;
>      case 's':
>          ssh_command = optarg;
>          break;
> @@ -7092,6 +7096,11 @@ int main_remus(int argc, char **argv)
>      domid = find_domain(argv[optind]);
>      host = argv[optind + 1];
>  
> +    if (r_info.netbuf) {
> +        if (default_remus_netbufscript)
> +            r_info.netbufscript = strdup(default_remus_netbufscript);

Can netbufscript be const and then drop the strdup?

> +    }
> +
>      if (r_info.blackhole) {
>          send_fd = open("/dev/null", O_RDWR, 0644);
>          if (send_fd < 0) {
> @@ -7126,13 +7135,10 @@ int main_remus(int argc, char **argv)
>      /* Point of no return */
>      rc = libxl_domain_remus_start(ctx, &r_info, domid, send_fd, recv_fd, 0);
>  
> -    /* If we are here, it means backup has failed/domain suspend failed.
> -     * Try to resume the domain and exit gracefully.
> +    /* If we are here, it means remus setup/domain susppend/backup has

"suspend"

Ian.