[lxc-devel] [PATCH] Protect global variables in log module via mutex

[Serge Hallyn]

Quoting S.Çağlar Onur (caglar at 10ur.org):
> Hi Serge,
> 
> On Mon, Nov 11, 2013 at 4:04 PM, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > Quoting S.Çağlar Onur (caglar at 10ur.org):
> >> Log module contains multiple global variables so protect them introducing a new mutex and serialize accessing log functions.
> >> Also gather all locking related code into src/lxc/lxclock.c
> >>
> >> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
> >
> > Really the log stuff should be re-thought.  What should happen right
> > now if two threads both call lxcapi_start() on containers with
> > lxc.logfile entries?  Perhaps we need two sets of log info.  One for
> > the program being used, and one for the running container.  Anything
> > done after src/lxc/start.c:lxc_start() logs to the container log info -
> > that's anyhthing relating to container setup, container monitor stuff,
> > hooks, and the running of the container.  Anything else is done to
> > the global log info - as that'll be shared by all threads.
> 
> Agreed.
> 
> > Hopefully someone finds this interesting enough to write a patch :)
> >
> > In the meantime - the infrastructure of this patch seems good, but
> > I don't think it really achieves protection of those variables.
> > log_fname and lxc_log_fd especially, because __lxc_log_set_file()
> > can close/free them concurrent with other __lxc_log_set_file()
> > runs and concurrent with lxc_log_get_file().
> >
> > What do you think would be the best way to achieve that?
> 
> Hmmm just an idea without giving lots of thought but considering the

Sorry, by 'to achieve that' i just meant to actually protect
log_fname and lxc_log_fd from stale accesses from another
thread after one thread has freed/closed them.  But,

> objective above what about storing those variables in container
> struct, adding a new method to API like c->log(c, MESSAGE, LEVEL) (or
> some helpers like APIERROR(c, MESSAGE), APIWARNING etc) and re-using
> parts of the log module there.
> 
> Come to think of it, do we really need to have a global/shared logging
> at all? What do you think making the whole logging thing to container
> specific?

That would solve the problem altogether :)  (Well, once we make sure
to access them only from under container_mem_lock()).

The first problem is that there are things that are done without us
being in the context of a container - especially when we're setting
up to create a container, or trying to get info about a container which
is not defined.

The second problem is that INFO(), ERROR() and friends will need to be
redefined to find the per-container logging info, which could be tough.

But I'll definately look at any patch trying to solve this.

-serge