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

[Serge Hallyn]

Quoting Christian Seiler (christian at iwakd.de):
> 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);

It's a tough line to draw.  We also don't want to have unwieldy blobs of
structs to manipulate for simple operations.  But I like your patch

>  - 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

And pid ns right?  Otherwise the task which did setns would look
funky inside the container.  iiuc.

>    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?

I think d).  Create a new attach_struct.h which you #include from both
lxccontainer.h and attach.h.

> 
> 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.
> 
> 
> ------------------------------------------------------------------------------
> AlienVault Unified Security Management (USM) platform delivers complete
> security visibility with the essential security capabilities. Easily and
> efficiently configure, manage, and operate all of your security controls
> from a single console and one unified framework. Download a free trial.
> http://p.sf.net/sfu/alienvault_d2d
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel