Re: [Xen-devel] [PATCH 5 of 6] libxl: refactor migrate_domain and generalize migrate_receive

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-01-31 at 01:05 +0000, rshriram@xxxxxxxxx wrote:
> # HG changeset patch
> # User Shriram Rajagopalan <rshriram@xxxxxxxxx>
> # Date 1327971533 28800
> # Node ID d79c7a853c644d459cda93bf61657be48104cd63
> # Parent  efe92d80c47487485056266a1404a8d2817b7f09
> 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@xxxxxxxxx>
> 
> diff -r efe92d80c474 -r d79c7a853c64 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c        Mon Jan 30 16:58:47 2012 -0800
> +++ b/tools/libxl/xl_cmdimpl.c        Mon Jan 30 16:58:53 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,18 +2653,23 @@ 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)
> +/* It is okay to have a NULL rune (we could be communicating over tcp sockets
> + * but not both NULL rune and NULL(send_fd, recv_fd).
> + */

The first check in the function ("Cannot create migration channel...")
seems to insist that both send_fd and recv_fd must be non-NULL without
reference to rune?

> +static void migrate_do_preamble(const char *domain_spec, const char *rune,
> +                                const char *override_config_file,
> +                                int *send_fd, int *recv_fd, pid_t *mchild)
>  {
> -    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;
> +    int rc = 0;
>      uint8_t *config_data;
>      int config_len;
> +    pid_t child = -1;
> +
> +    if (!send_fd || !recv_fd) {
> +        fprintf(stderr, "Cannot create migration channel:"
> +                "Null file descriptors supplied!\n");
> +        exit(1);
> +    }
>  
>      save_domain_core_begin(domain_spec, override_config_file,
>                             &config_data, &config_len);
> @@ -2638,43 +2680,44 @@ static void migrate_domain(const char *d
[...]
> +    if (*send_fd < 0 || *recv_fd < 0) {
> +        if (rune)
> +            child = create_migration_child(rune, send_fd, recv_fd);
> +        if (child < 0) {
> +            fprintf(stderr, "failed to create migration channel"
> +                    " - empty command ?\n");
> +            exit(1);
> +        }
> +    }

I think the migrate_domain function should contain:
        save_domain_core_begin
        create_migration_child(&rx_fd, &tx_fd) (or int fd[2])
        migrate_do_preamble(rx_fd, tx_fd) 

And that migrate_do_preamble should start at this point:

> +
> +    rc = migrate_read_fixedmessage(*recv_fd, migrate_receiver_banner,
>                                     sizeof(migrate_receiver_banner)-1,
>                                     "banner", rune);

Rather than trying to push the create_migration_child down into
migrate_do_preamble. I think that gives us sufficient building blocks
and shared code to do what you want or to add tcp support later etc.

> @@ -2798,7 +2841,12 @@ 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)
> +/* Explicitly specifying a send & recv fd allows us to switch to a tcp socket
> + * based migration/replication channel in future, instead of exec/forking 
> from
> + * an ssh channel.
> + */

I don't think you need to justify the use of send_fd/recv_fd in the
comment here, it seems natural to allow the caller to pass in the
communication fds for whatever reason and the commit message is a fine
place to mention your specific reasons for wanting it.

[...]
> @@ -2983,7 +3031,8 @@ int main_migrate_receive(int argc, char 
>          help("migrate-receive");
>          return 2;
>      }
> -    migrate_receive(debug, daemonize, monitor);
> +    migrate_receive(debug, daemonize, monitor,
> +                    /* write to stdout */1, /* read from stdin */0);

unistd.h defines STD{IN,OUT,ERR}_FILENO which would be useful here.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel