Re: [Xen-devel] [PATCH 06/21] libxl: domain save/restore: run in a separate process

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

> [...]
> +/* stream_fd is as from the caller (eventually, the application).
> + * It may be 0, 1 or 2, in which case we need to dup it elsewhere.
> + * The actual fd value is not included in the supplied argnums; rather
> + * it will be automatically supplied by run_helper as the 2nd argument.
> + *
> + * preserve_fds are fds that the caller is intending to pass to the
> + * helper so which need cloexec clearing.  They may not be 0, 1 or 2.
> + * An entry may be -1 in which case it will be ignored.
[...]
> +static void run_helper(libxl__egc *egc, libxl__save_helper_state *shs,
> +                       const char *mode_arg, int stream_fd,
> +                       const int *preserve_fds, int num_preserve_fds,
> +                       const unsigned long *argnums, int num_argnums)
> +{
> +    STATE_AO_GC(shs->ao);
> +    const char *args[4 + num_argnums];
> +    const char **arg = args;
> +    int i, rc;
> +
> +    /* Resources we must free */
> +    libxl__carefd *childs_pipes[2] = { 0,0 };
> +
> +    /* Convenience aliases */
> +    const uint32_t domid = shs->domid;
> +
> +    shs->rc = 0;
> +    shs->completed = 0;
> +    shs->pipes[0] = shs->pipes[1] = 0;
> +    libxl__ev_fd_init(&shs->readable);
> +    libxl__ev_child_init(&shs->child);
> +
> +    shs->stdin_what = GCSPRINTF("domain %"PRIu32" save/restore helper"
> +                                " stdin pipe", domid);
> +    shs->stdout_what = GCSPRINTF("domain %"PRIu32" save/restore helper"
> +                                 " stdout pipe", domid);
> +
> +    *arg++ = getenv("LIBXL_SAVE_HELPER") ?: LIBEXEC "/" "libxl-save-helper";
> +    *arg++ = mode_arg;
> +    const char **stream_fd_arg = arg++;
> +    for (i=0; i<num_argnums; i++)
> +        *arg++ = GCSPRINTF("%lu", argnums[i]);
> +    *arg++ = 0;
> +    assert(arg == args + ARRAY_SIZE(args));
> +
> +    libxl__carefd_begin();
> +    int childfd;
> +    for (childfd=0; childfd<2; childfd++) {
> +        /* Setting up the pipe for the child's fd childfd */
> +        int fds[2];
> +        if (libxl_pipe(CTX,fds)) { rc = ERROR_FAIL; goto out; }
> +        int childs_end = childfd==0 ? 0 /*read*/  : 1 /*write*/;
> +        int our_end    = childfd==0 ? 1 /*write*/ : 0 /*read*/;
> +        childs_pipes[childfd] = libxl__carefd_record(CTX, fds[childs_end]);
> +        shs->pipes[childfd] =   libxl__carefd_record(CTX, fds[our_end]);
> +    }
> +    libxl__carefd_unlock();
> +
> +    pid_t pid = libxl__ev_child_fork(gc, &shs->child, helper_exited);
> +    if (!pid) {
> +        if (stream_fd <= 2) {
> +            stream_fd = dup(stream_fd);
> +            if (stream_fd < 0) {
> +                LOGE(ERROR,"dup migration stream fd");

LOGE uses CTX -- is that safe after fork, and will it go anywhere
anyway?

> +                exit(-1);
> +            }
> +        }
> +        libxl_fd_set_cloexec(CTX, stream_fd, 0);
> +        *stream_fd_arg = GCSPRINTF("%d", stream_fd);
> +
> +        for (i=0; i<num_preserve_fds; i++)
> +            if (preserve_fds[i] >= 0)
> +                libxl_fd_set_cloexec(CTX, preserve_fds[i], 0);

The doc comment says these cannot be 0,1,2. Possibly not worth enforcing
though.

Other than those two nits:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel