Re: [PATCH 5 of 6 V3] libxl: refactor migrate_domain and generalize migrate_receive

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

On Fri, 2012-02-03 at 06:50 +0000, rshriram@cs.ubc.ca wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@cs.ubc.ca>
> # Date 1328251593 28800
> # Node ID 62c4fd2fe9bbc2c283e3998d852317a48e9f9770
> # Parent  f853c88f0230a2e9d2e1006a9cd220c4cd27e74d
> libxl: refactor migrate_domain and generalize migrate_receive
> 
> Refactor some tasks like establishing the migration channel,
> initial migration protocol exchange into separate functions,
> to facilitate re-use, when remus support is introduced. Also,
> make migrate_receive generic (instead of resorting to stdin and
> stdout as the file descriptors for communication).
> 
> Signed-off-by: Shriram Rajagopalan <rshriram@cs.ubc.ca>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> diff -r f853c88f0230 -r 62c4fd2fe9bb tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Feb 02 22:46:33 2012 -0800
> +++ b/tools/libxl/xl_cmdimpl.c	Thu Feb 02 22:46:33 2012 -0800
> @@ -2531,6 +2531,43 @@ static int save_domain(const char *p, co
>      exit(0);
>  }
>  
> +static pid_t create_migration_child(const char *rune, int *send_fd,
> +                                        int *recv_fd)
> +{
> +    int sendpipe[2], recvpipe[2];
> +    pid_t child = -1;
> +
> +    if (!rune || !send_fd || !recv_fd)
> +        return -1;
> +
> +    MUST( libxl_pipe(ctx, sendpipe) );
> +    MUST( libxl_pipe(ctx, recvpipe) );
> +
> +    child = libxl_fork(ctx);
> +    if (child==-1) exit(1);
> +
> +    if (!child) {
> +        dup2(sendpipe[0], 0);
> +        dup2(recvpipe[1], 1);
> +        close(sendpipe[0]); close(sendpipe[1]);
> +        close(recvpipe[0]); close(recvpipe[1]);
> +        execlp("sh","sh","-c",rune,(char*)0);
> +        perror("failed to exec sh");
> +        exit(-1);
> +    }
> +
> +    close(sendpipe[0]);
> +    close(recvpipe[1]);
> +    *send_fd = sendpipe[1];
> +    *recv_fd = recvpipe[0];
> +
> +    /* if receiver dies, we get an error and can clean up
> +       rather than just dying */
> +    signal(SIGPIPE, SIG_IGN);
> +
> +    return child;
> +}
> +
>  static int migrate_read_fixedmessage(int fd, const void *msg, int msgsz,
>                                       const char *what, const char *rune) {
>      char buf[msgsz];
> @@ -2616,53 +2653,17 @@ static void migration_child_report(pid_t
>      migration_child = 0;
>  }
>  
> -static void migrate_domain(const char *domain_spec, const char *rune,
> -                           const char *override_config_file)
> +static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
> +                                uint8_t *config_data, int config_len,
> +                                const char *rune)
>  {
> -    pid_t child = -1;
> -    int rc;
> -    int sendpipe[2], recvpipe[2];
> -    int send_fd, recv_fd;
> -    libxl_domain_suspend_info suspinfo;
> -    char *away_domname;
> -    char rc_buf;
> -    uint8_t *config_data;
> -    int config_len;
> -
> -    save_domain_core_begin(domain_spec, override_config_file,
> -                           &config_data, &config_len);
> -
> -    if (!config_len) {
> -        fprintf(stderr, "No config file stored for running domain and "
> -                "none supplied - cannot migrate.\n");
> +    int rc = 0;
> +
> +    if (send_fd < 0 || recv_fd < 0) {
> +        fprintf(stderr, "migrate_do_preamble: invalid file descriptors\n");
>          exit(1);
>      }
>  
> -    MUST( libxl_pipe(ctx, sendpipe) );
> -    MUST( libxl_pipe(ctx, recvpipe) );
> -
> -    child = libxl_fork(ctx);
> -    if (child==-1) exit(1);
> -
> -    if (!child) {
> -        dup2(sendpipe[0], 0);
> -        dup2(recvpipe[1], 1);
> -        close(sendpipe[0]); close(sendpipe[1]);
> -        close(recvpipe[0]); close(recvpipe[1]);
> -        execlp("sh","sh","-c",rune,(char*)0);
> -        perror("failed to exec sh");
> -        exit(-1);
> -    }
> -
> -    close(sendpipe[0]);
> -    close(recvpipe[1]);
> -    send_fd = sendpipe[1];
> -    recv_fd = recvpipe[0];
> -
> -    signal(SIGPIPE, SIG_IGN);
> -    /* if receiver dies, we get an error and can clean up
> -       rather than just dying */
> -
>      rc = migrate_read_fixedmessage(recv_fd, migrate_receiver_banner,
>                                     sizeof(migrate_receiver_banner)-1,
>                                     "banner", rune);
> @@ -2675,6 +2676,34 @@ static void migrate_domain(const char *d
>      save_domain_core_writeconfig(send_fd, "migration stream",
>                                   config_data, config_len);
>  
> +}
> +
> +static void migrate_domain(const char *domain_spec, const char *rune,
> +                           const char *override_config_file)
> +{
> +    pid_t child = -1;
> +    int rc;
> +    int send_fd = -1, recv_fd = -1;
> +    libxl_domain_suspend_info suspinfo;
> +    char *away_domname;
> +    char rc_buf;
> +    uint8_t *config_data;
> +    int config_len;
> +
> +    save_domain_core_begin(domain_spec, override_config_file,
> +                           &config_data, &config_len);
> +
> +    if (!config_len) {
> +        fprintf(stderr, "No config file stored for running domain and "
> +                "none supplied - cannot migrate.\n");
> +        exit(1);
> +    }
> +
> +    child = create_migration_child(rune, &send_fd, &recv_fd);
> +
> +    migrate_do_preamble(send_fd, recv_fd, child, config_data, config_len,
> +                        rune);
> +
>      xtl_stdiostream_adjust_flags(logger, XTL_STDIOSTREAM_HIDE_PROGRESS, 0);
>  
>      memset(&suspinfo, 0, sizeof(suspinfo));
> @@ -2798,7 +2827,8 @@ static void core_dump_domain(const char 
>      if (rc) { fprintf(stderr,"core dump failed (rc=%d)\n",rc);exit(-1); }
>  }
>  
> -static void migrate_receive(int debug, int daemonize, int monitor)
> +static void migrate_receive(int debug, int daemonize, int monitor,
> +                            int send_fd, int recv_fd)
>  {
>      int rc, rc2;
>      char rc_buf;
> @@ -2810,7 +2840,7 @@ static void migrate_receive(int debug, i
>  
>      fprintf(stderr, "migration target: Ready to receive domain.\n");
>  
> -    CHK_ERRNO( libxl_write_exactly(ctx, 1,
> +    CHK_ERRNO( libxl_write_exactly(ctx, send_fd,
>                                     migrate_receiver_banner,
>                                     sizeof(migrate_receiver_banner)-1,
>                                     "migration ack stream",
> @@ -2822,7 +2852,7 @@ static void migrate_receive(int debug, i
>      dom_info.monitor = monitor;
>      dom_info.paused = 1;
>      dom_info.restore_file = "incoming migration stream";
> -    dom_info.migrate_fd = 0; /* stdin */
> +    dom_info.migrate_fd = recv_fd;
>      dom_info.migration_domname_r = &migration_domname;
>      dom_info.no_incr_generationid = 1;
>  
> @@ -2836,13 +2866,13 @@ static void migrate_receive(int debug, i
>      fprintf(stderr, "migration target: Transfer complete,"
>              " requesting permission to start domain.\n");
>  
> -    rc = libxl_write_exactly(ctx, 1,
> +    rc = libxl_write_exactly(ctx, send_fd,
>                               migrate_receiver_ready,
>                               sizeof(migrate_receiver_ready),
>                               "migration ack stream", "ready message");
>      if (rc) exit(-rc);
>  
> -    rc = migrate_read_fixedmessage(0, migrate_permission_to_go,
> +    rc = migrate_read_fixedmessage(recv_fd, migrate_permission_to_go,
>                                     sizeof(migrate_permission_to_go),
>                                     "GO message", 0);
>      if (rc) goto perhaps_destroy_notify_rc;
> @@ -2861,7 +2891,7 @@ static void migrate_receive(int debug, i
>      rc = 0;
>  
>   perhaps_destroy_notify_rc:
> -    rc2 = libxl_write_exactly(ctx, 1,
> +    rc2 = libxl_write_exactly(ctx, send_fd,
>                                migrate_report, sizeof(migrate_report),
>                                "migration ack stream",
>                                "success/failure report");
> @@ -2869,7 +2899,7 @@ static void migrate_receive(int debug, i
>  
>      rc_buf = -rc;
>      assert(!!rc_buf == !!rc);
> -    rc2 = libxl_write_exactly(ctx, 1, &rc_buf, 1,
> +    rc2 = libxl_write_exactly(ctx, send_fd, &rc_buf, 1,
>                                "migration ack stream",
>                                "success/failure code");
>      if (rc2) exit(-ERROR_BADFAIL);
> @@ -2887,7 +2917,7 @@ static void migrate_receive(int debug, i
>          fprintf(stderr, "migration target: Cleanup OK, granting sender"
>                  " permission to resume.\n");
>  
> -        rc2 = libxl_write_exactly(ctx, 1,
> +        rc2 = libxl_write_exactly(ctx, send_fd,
>                                    migrate_permission_to_go,
>                                    sizeof(migrate_permission_to_go),
>                                    "migration ack stream",
> @@ -2983,7 +3013,9 @@ int main_migrate_receive(int argc, char 
>          help("migrate-receive");
>          return 2;
>      }
> -    migrate_receive(debug, daemonize, monitor);
> +    migrate_receive(debug, daemonize, monitor,
> +                    STDOUT_FILENO, STDIN_FILENO);
> +
>      return 0;
>  }
>