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

[Stéphane Graber]

On Fri, Jan 03, 2014 at 06:22:11PM -0500, S.Çağlar Onur wrote:
> Hi,
> 
> On Fri, Jan 3, 2014 at 4:32 PM, Stéphane Graber <stgraber at ubuntu.com> wrote:
> > 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.
> 
> I think new version at http://10ur.org/lock.patch addresses both yours
> and Serge's comments (I hate gmail for making me upload stuff instead
> of pasting inline). Let me know what you think so that I can send it
> as a proper patch.

Looks good to me. Thanks.

> 
> > --
> > Stéphane Graber
> > Ubuntu developer
> > http://www.ubuntu.com
> >
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> >
> 
> 
> 
> -- 
> S.Çağlar Onur <caglar at 10ur.org>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel

-- 
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/78a515d8/attachment.pgp>