[lxc-devel] [PATCH] Use pthread_atfork() to unlock mutexes after fork()

[Stéphane Graber]

On Fri, Jan 03, 2014 at 04:22:51PM -0500, S.Çağlar Onur wrote:
> Hi,
> 
> On Fri, Jan 3, 2014 at 3:13 PM, Stéphane Graber <stgraber at ubuntu.com> wrote:
> > On Thu, Jan 02, 2014 at 08:59:10AM -0600, Serge Hallyn wrote:
> >> Quoting Stéphane Graber (stgraber at ubuntu.com):
> >> > On Wed, Jan 01, 2014 at 11:37:32PM -0600, Serge Hallyn wrote:
> >> > > Quoting Stéphane Graber (stgraber at ubuntu.com):
> >> > > > This patch caused a build failure on Android:
> >> > > >
> >> > > > arm-linux-androideabi-gcc --sysroot=/tmp/android-build-scripts/android-ndk-r9b/platforms/android-9/arch-arm/ -DHAVE_CONFIG_H -I. -I../../src    -fPIC -DPIC -I../../src -DLXCROOTFSMOUNT=\"/data/lxc/lxc/lib/lxc/rootfs\" -DLXCPATH=\"/data/lxc/lxc/var/lib/lxc\" -DLXC_GLOBAL_CONF=\"/data/lxc/lxc/etc/lxc/lxc.conf\" -DLXCINITDIR=\"/data/lxc/lxc/libexec\" -DLXCTEMPLATEDIR=\"/data/lxc/lxc/share/lxc/templates\" -DLOGPATH=\"/data/lxc/lxc/var/log/lxc\" -DLXC_DEFAULT_CONFIG=\"/data/lxc/lxc/etc/lxc/default.conf\" -DLXClxclock.c: In function 'process_lock_setup_atfork':
> >> > > > lxclock.c:332:2: error: implicit declaration of function 'pthread_atfork' [-Werror=implicit-function-declaration]
> >> > > >   pthread_atfork(process_lock, process_unlock, process_unlock);
> >> > >
> >> > > Hm,  according to
> >> > > http://stackoverflow.com/questions/12370970/undefined-reference-to-pthread-atfork-while-i-was-trying-to-port-libpcsclite-t
> >> > > perhaps all we need is to declare the fn before its use.  Do you
> >> > > have a local bionic compiler handy to test that on?
> >> >
> >> > Not having much luck with that... this just turned it into:
> >> >
> >> > liblxc.so: error: undefined reference to 'pthread_atfork'
> >> >
> >> > pthread on bionic is part of the C library so no extra linker flags
> >> > should be needed to make this work...
> >>
> >> Hm.  So we can either (a) keep looking for a way to make it work in
> >> bionic, (b) give in and switch all uses of fork() to lxc_fork() to
> >> do our own atfork hook, or (c) detect at configure.ac whether we have
> >> pthread_atfork(), and if not then just don't do it.
> >
> > I think doing (c) would be the easiest here and would also mean that
> > whenever pthread_atfork actually shows up in bionic (assuming the
> > configure.ac detection code doesn't already detect it as present...),
> > things will just work.
> >
> > It may be worth adding an entry to the configure.ac config overview part
> > to indicate threading support so it's clear to anyone building on bionic
> > that they shouldn't expect this to be working.
> 
> What about something like [1] (after reverting
> 64b1be2903078ef9e9ba3ffcbc30a4dc9bc5cc6c)?
> 
> It adds the pthread_atfork check, removes static_lock/unlock, puts the
> values array into TLS for non-bionic targets and lastly add a warning
> for bionic users.
> 
> [1] http://10ur.org/lock.patch

Hmm, why use IS_BIONIC at some places and HAVE_PTHREAD_ATFORK at others?

I'm not familar at all with pthread but assuming the two ifdefed chunk
of code are interdependent, having both of them under the same condition
would make more sense to me.

I also would love to eventually get rid of IS_BIONIC/is_bionic and just
rely on feature checks so I think it'd be best to have the warning
issued if any of the pthread related checks fail (whatever the C library
in use) and have lxclock.c check the feature flags (or introduce a new
HAS_THREADS define if easier) instead of looking for bionic.

-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140103/101f167f/attachment-0001.pgp>