[lxc-devel] [PATCH] [RFC] Complete rewrite of lxc-attach functionality

[Christian Seiler]

Hi,

as discussed previously on this list, I've reimplemented the lxc-attach
functionality as an API function. The patchset consists of two parts:

 1. Four minor patches that just fix some bugs, shuffle definitions
    around and implement some small utility functions that I need for
    the attach functionality, but which also may be useful generally.

    These patches are trivial and I've signed off on them, I don't see
    any reason not for applying them anyway.

 2. One major patch that actually reimplements the lxc-attach
    functionality in form of a function lxc_attach(). Since I want some
    feedback here, I haven't added the Signed-off-by tag to the commit
    message yet and I don't think it's quite ready to be committed yet.

Rationale behind the design of the new functionality:

 - We have a ton of knobs one could want to do while attaching, my idea
   was to give the API user the largest possible control over all of these
   knobs. But since I didn't want to run into the trap of creating a
   function that accepts 100 parameters, I thought the best idea would
   be to have a structure (lxc_attach_options_t) where one specifies
   precisely all of the options and just passes this structure to the
   function. This way, lxc_attach only has 4 parameters and is called
   like this:

      ret = lxc_attach(container_name, lxcpath, &options_struct, &pid);

 - In 0.9, we have a three-process hierarchy:

      lxc-attach
        +- lxc-attach [does setns() before spawning further]
            +- attached process

   This is due to user namespaces. The patch introduces a flat
   hierarchy thanks to CLONE_PARENT:

      lxc-attach
        +- lxc-attach [does setns(), dies after creating next process]
        +- attached process

   That way, once attach is complete, there are two proceses running:
   lxc-attach (supervising) and the attached process itself.

 - In order to be able to do attach safely from a threaded program, we
   have to make sure that all fds (including ipc sockets) that we open
   do not conflict with other threads. For this reason, I use O_CLOEXEC
   for all fds. That way, if another thread fork()s off while an attach
   is in progress, as soon as that other process exec()s, the fd will
   be closed and thus not leak. However, if a process that has been
   forked from another thread does not exec(), then we run into the
   problem that the process calling attach usually notices that error
   by means of detecting that the IPC socket was closed unexpectedly.
   Since another process that forked away at the wrong time from a
   diffrerent thread may still hold an open fd of the child process's
   socket, the child will call shutdown() on its socket before closing
   it in order to make sure the parent still notices the end of the
   socket. [1]

The functionality itself is tested, lxc-attach still works the same as
before.

Now for the part that I don't like about my current implementation: Once
I was done with adding the lxc_attach() function, I wanted to update the
"general purpose" LXC API, including Python bindings etc.. But for this
I would also have to have the struct lxc_attach_options_t in lxc_attach,
which would mean including attach.h from lxccontainer.h. But attach.h
contains all sorts of very low-level functions and lxccontainer.h is
supposed to be a high-level API.

Now I'm not quite sure what to do:

 a) Add a really dumbed-down high-level API function that just accepts
    the name of a program to start. (Kind of defeats the purpose of
    having the container API.)

 b) Really do include attach.h from lxccontainer.h (which currently
    does not include any other lxc header!)

 c) Move lxc_attach_options_t from attach.h to lxccontainer.h, include
    lxccontainer.h from attach.h, add a function with the respective
    signature to the container API and let that call lxc_attach()
    internally. (My current favorite.)

 d) Something else?

Thoughts? On this specific issue and also on the general
implementation?

-- Christian

[1] Note that this is still not safe from an interruption by a signal,
    I will provide a separate patch that addresses that once the main
    functionality has been merged into staging.