Quoting Stéphane Graber ([email protected]): > 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? > ^ > cc1: all warnings being treated as errors > make[3]: *** [liblxc_so-lxclock.o] Error 1 > make[3]: Leaving directory `/tmp/android-build-scripts/lxc/src/lxc' > make[2]: *** [all-recursive] Error 1 > make[2]: Leaving directory `/tmp/android-build-scripts/lxc/src' > make[1]: *** [all] Error 2 > make[1]: Leaving directory `/tmp/android-build-scripts/lxc/src' > make: *** [all-recursive] Error 1 > > It looks like pthread_atfork doesn't exist in bionic. > > On Wed, Jan 01, 2014 at 01:52:33PM -0600, Serge Hallyn wrote: > > Quoting Andrey Mazo ([email protected]): > > > Signed-off-by: Andrey Mazo <[email protected]> > > > --- > > > src/lxc/Makefile.am | 3 ++- > > > src/lxc/attach.c | 2 -- > > > src/lxc/bdev.c | 13 ------------- > > > src/lxc/lxccontainer.c | 4 ---- > > > src/lxc/lxclock.c | 23 ++++++++++++++++++++++- > > > src/lxc/monitor.c | 1 - > > > 6 files changed, 24 insertions(+), 22 deletions(-) > > > > > > diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am > > > index 74b38e2..f18d6e1 100644 > > > --- a/src/lxc/Makefile.am > > > +++ b/src/lxc/Makefile.am > > > @@ -135,9 +135,10 @@ AM_CFLAGS += -DHAVE_SECCOMP > > > liblxc_so_SOURCES += seccomp.c > > > endif > > > > > > -liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS) > > > +liblxc_so_CFLAGS = -fPIC -DPIC $(AM_CFLAGS) -pthread > > > > > > liblxc_so_LDFLAGS = \ > > > + -pthread \ > > > -shared \ > > > -Wl,-soname,liblxc.so.$(firstword $(subst ., ,$(VERSION))) > > > > > > diff --git a/src/lxc/attach.c b/src/lxc/attach.c > > > index 6749617..252e172 100644 > > > --- a/src/lxc/attach.c > > > +++ b/src/lxc/attach.c > > > @@ -497,7 +497,6 @@ char *lxc_attach_getpwshell(uid_t uid) > > > NULL > > > }; > > > > > > - process_unlock(); // we're no longer sharing > > > close(pipes[0]); > > > > > > /* we want to capture stdout */ > > > @@ -790,7 +789,6 @@ int lxc_attach(const char* name, const char* lxcpath, > > > lxc_attach_exec_t exec_fun > > > return -1; > > > } > > > > > > - process_unlock(); // we're no longer sharing > > > /* first subprocess begins here, we close the socket that is for the > > > * initial thread > > > */ > > > diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c > > > index b737eff..ec0d0ee 100644 > > > --- a/src/lxc/bdev.c > > > +++ b/src/lxc/bdev.c > > > @@ -72,7 +72,6 @@ static int do_rsync(const char *src, const char *dest) > > > if (pid > 0) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > l = strlen(src) + 2; > > > s = malloc(l); > > > if (!s) > > > @@ -197,7 +196,6 @@ static int do_mkfs(const char *path, const char > > > *fstype) > > > if (pid > 0) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > // If the file is not a block device, we don't want mkfs to ask > > > // us about whether to proceed. > > > close(0); > > > @@ -282,7 +280,6 @@ static int detect_fs(struct bdev *bdev, char *type, > > > int len) > > > return ret; > > > } > > > > > > - process_unlock(); // we're no longer sharing > > > if (unshare(CLONE_NEWNS) < 0) > > > exit(1); > > > > > > @@ -574,7 +571,6 @@ static int zfs_clone(const char *opath, const char > > > *npath, const char *oname, > > > if (!pid) { > > > char dev[MAXPATHLEN]; > > > > > > - process_unlock(); // we're no longer sharing > > > ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, > > > nname); > > > if (ret < 0 || ret >= MAXPATHLEN) > > > exit(1); > > > @@ -598,7 +594,6 @@ static int zfs_clone(const char *opath, const char > > > *npath, const char *oname, > > > if ((pid = fork()) < 0) > > > return -1; > > > if (!pid) { > > > - process_unlock(); // we're no longer sharing > > > execlp("zfs", "zfs", "destroy", path1, NULL); > > > exit(1); > > > } > > > @@ -609,7 +604,6 @@ static int zfs_clone(const char *opath, const char > > > *npath, const char *oname, > > > if ((pid = fork()) < 0) > > > return -1; > > > if (!pid) { > > > - process_unlock(); // we're no longer sharing > > > execlp("zfs", "zfs", "snapshot", path1, NULL); > > > exit(1); > > > } > > > @@ -620,7 +614,6 @@ static int zfs_clone(const char *opath, const char > > > *npath, const char *oname, > > > if ((pid = fork()) < 0) > > > return -1; > > > if (!pid) { > > > - process_unlock(); // we're no longer sharing > > > execlp("zfs", "zfs", "clone", option, path1, path2, > > > NULL); > > > exit(1); > > > } > > > @@ -671,7 +664,6 @@ static int zfs_destroy(struct bdev *orig) > > > if (pid) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > if (!zfs_list_entry(orig->src, output, MAXPATHLEN)) { > > > ERROR("Error: zfs entry for %s not found", orig->src); > > > return -1; > > > @@ -716,7 +708,6 @@ static int zfs_create(struct bdev *bdev, const char > > > *dest, const char *n, > > > if (pid) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > char dev[MAXPATHLEN]; > > > ret = snprintf(dev, MAXPATHLEN, "%s/%s", zfsroot, n); > > > if (ret < 0 || ret >= MAXPATHLEN) > > > @@ -861,7 +852,6 @@ static int do_lvm_create(const char *path, unsigned > > > long size, const char *thinp > > > if (pid > 0) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > // lvcreate default size is in M, not bytes. > > > ret = snprintf(sz, 24, "%lu", size/1000000); > > > if (ret < 0 || ret >= 24) > > > @@ -921,7 +911,6 @@ static int lvm_snapshot(const char *orig, const char > > > *path, unsigned long size) > > > if (pid > 0) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > // lvcreate default size is in M, not bytes. > > > ret = snprintf(sz, 24, "%lu", size/1000000); > > > if (ret < 0 || ret >= 24) > > > @@ -1055,7 +1044,6 @@ static int lvm_destroy(struct bdev *orig) > > > if ((pid = fork()) < 0) > > > return -1; > > > if (!pid) { > > > - process_unlock(); // we're no longer sharing > > > execlp("lvremove", "lvremove", "-f", orig->src, NULL); > > > exit(1); > > > } > > > @@ -2091,7 +2079,6 @@ struct bdev *bdev_copy(const char *src, const char > > > *oldname, const char *cname, > > > return new; > > > } > > > > > > - process_unlock(); // we're no longer sharing > > > if (unshare(CLONE_NEWNS) < 0) { > > > SYSERROR("unshare CLONE_NEWNS"); > > > exit(1); > > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c > > > index e1d004f..cf71f28 100644 > > > --- a/src/lxc/lxccontainer.c > > > +++ b/src/lxc/lxccontainer.c > > > @@ -599,7 +599,6 @@ static bool lxcapi_start(struct lxc_container *c, int > > > useinit, char * const argv > > > if (pid != 0) > > > return wait_on_daemonized_start(c, pid); > > > > > > - process_unlock(); // we're no longer sharing > > > /* second fork to be reparented by init */ > > > pid = fork(); > > > if (pid < 0) { > > > @@ -837,7 +836,6 @@ static bool create_run_template(struct lxc_container > > > *c, char *tpath, bool quiet > > > char **newargv; > > > struct lxc_conf *conf = c->lxc_conf; > > > > > > - process_unlock(); // we're no longer sharing > > > if (quiet) { > > > close(0); > > > close(1); > > > @@ -1234,7 +1232,6 @@ static bool lxcapi_create(struct lxc_container *c, > > > const char *t, > > > if (pid == 0) { // child > > > struct bdev *bdev = NULL; > > > > > > - process_unlock(); // we're no longer sharing > > > if (!(bdev = do_bdev_create(c, bdevtype, specs))) { > > > ERROR("Error creating backing store type %s for %s", > > > bdevtype ? bdevtype : "(none)", c->name); > > > @@ -2326,7 +2323,6 @@ static int clone_update_rootfs(struct lxc_container > > > *c0, > > > if (pid > 0) > > > return wait_for_pid(pid); > > > > > > - process_unlock(); // we're no longer sharing > > > bdev = bdev_init(c->lxc_conf->rootfs.path, c->lxc_conf->rootfs.mount, > > > NULL); > > > if (!bdev) > > > exit(1); > > > diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c > > > index 28691b0..36c4bd3 100644 > > > --- a/src/lxc/lxclock.c > > > +++ b/src/lxc/lxclock.c > > > @@ -87,7 +87,7 @@ void unlock_mutex(pthread_mutex_t *l) > > > int ret; > > > > > > if ((ret = pthread_mutex_unlock(l)) != 0) { > > > - fprintf(stderr, "pthread_mutex_lock returned:%d %s", ret, > > > strerror(ret)); > > > + fprintf(stderr, "pthread_mutex_unlock returned:%d %s", ret, > > > strerror(ret)); > > > dump_stacktrace(); > > > exit(1); > > > } > > > @@ -315,6 +315,21 @@ void process_unlock(void) > > > unlock_mutex(&thread_mutex); > > > } > > > > > > +/* One thread can do fork() while another one is holding a mutex. > > > + * There is only one thread in child just after the fork(), so noone > > > will ever release that mutex. > > > + * We setup a "child" fork handler to unlock the mutex just after the > > > fork(). > > > + * For several mutex types, unlocking an unlocked mutex can lead to > > > undefined behavior. > > > > Thanks for pointing this out. I have to say I don't like that we have > > to lock this... though it should be safe with our current usage (and > > I see Caglar has verified it :) > > > > An alternative would be to have a unlock_if_locked() for each lock, > > and do pthread_atfork(NULL, NULL, process_unlock_if_locked()). > > There would be no risk of a race since the child is single-threaded. > > > > But this is ok for now. > > > > Acked-by: Serge E. Hallyn <[email protected]> > > > > (Note I'm acking, but not applying until tomorrow) > > > > > + * One way to deal with it is to setup "prepare" fork handler > > > + * to lock the mutex before fork() and both "parent" and "child" fork > > > handlers > > > + * to unlock the mutex. > > > + * This forbids doing fork() while explicitly holding the lock. > > > + */ > > > +__attribute__((constructor)) > > > +static void process_lock_setup_atfork(void) > > > +{ > > > + pthread_atfork(process_lock, process_unlock, process_unlock); > > > +} > > > + > > > /* Protects static const values inside the lxc_global_config_value > > > funtion */ > > > void static_lock(void) > > > { > > > @@ -326,6 +341,12 @@ void static_unlock(void) > > > unlock_mutex(&static_mutex); > > > } > > > > > > +__attribute__((constructor)) > > > +static void static_lock_setup_atfork(void) > > > +{ > > > + pthread_atfork(static_lock, static_unlock, static_unlock); > > > +} > > > + > > > int container_mem_lock(struct lxc_container *c) > > > { > > > return lxclock(c->privlock, 0); > > > diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c > > > index 7e0a713..0b6a065 100644 > > > --- a/src/lxc/monitor.c > > > +++ b/src/lxc/monitor.c > > > @@ -300,7 +300,6 @@ int lxc_monitord_spawn(const char *lxcpath) > > > return 0; > > > } > > > > > > - process_unlock(); // we're no longer sharing > > > if (pipe(pipefd) < 0) { > > > SYSERROR("failed to create pipe"); > > > exit(EXIT_FAILURE); > > > -- > > > 1.8.4.5 > > > > > > _______________________________________________ > > > lxc-devel mailing list > > > [email protected] > > > http://lists.linuxcontainers.org/listinfo/lxc-devel > > _______________________________________________ > > lxc-devel mailing list > > [email protected] > > http://lists.linuxcontainers.org/listinfo/lxc-devel > > -- > Stéphane Graber > Ubuntu developer > http://www.ubuntu.com > _______________________________________________ > lxc-devel mailing list > [email protected] > http://lists.linuxcontainers.org/listinfo/lxc-devel _______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
