summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorZbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>2014-07-18 20:12:13 -0400
committerZbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>2014-07-18 20:12:44 -0400
commit7566e26721ee95d6fc864e9e6654fb61bd3cd603 (patch)
tree4ac5f457cee12ccbea45f5c90eabe76cbd1444be
parentcbd4560ea2c9f0ae77df1fc64685ff4e559810b6 (diff)
barrier: initalize file descriptors with -1
Explicitly initalize descriptors using explicit assignment like bus_error. This makes barriers follow the same conventions as everything else and makes things a bit simpler too. Rename barier_init to barier_create so it is obvious that it is not about initialization. Remove some parens, etc.
-rw-r--r--src/nspawn/nspawn.c4
-rw-r--r--src/shared/barrier.c114
-rw-r--r--src/shared/barrier.h4
-rw-r--r--src/shared/pty.c3
-rw-r--r--src/test/test-barrier.c4
5 files changed, 50 insertions, 79 deletions
diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 657512d661..7c47f6ecd2 100644
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -3073,13 +3073,13 @@ int main(int argc, char *argv[]) {
for (;;) {
ContainerStatus container_status;
- _cleanup_(barrier_destroy) Barrier barrier = { };
+ _cleanup_(barrier_destroy) Barrier barrier = BARRIER_NULL;
struct sigaction sa = {
.sa_handler = nop_handler,
.sa_flags = SA_NOCLDSTOP,
};
- r = barrier_init(&barrier);
+ r = barrier_create(&barrier);
if (r < 0) {
log_error("Cannot initialize IPC barrier: %s", strerror(-r));
goto finish;
diff --git a/src/shared/barrier.c b/src/shared/barrier.c
index c198329cb0..4ac42d0239 100644
--- a/src/shared/barrier.c
+++ b/src/shared/barrier.c
@@ -93,7 +93,7 @@
*/
/**
- * barrier_init() - Initialize a barrier object
+ * barrier_create() - Initialize a barrier object
* @obj: barrier to initialize
*
* This initializes a barrier object. The caller is responsible of allocating
@@ -111,26 +111,16 @@
*
* Returns: 0 on success, negative error code on failure.
*/
-int barrier_init(Barrier *obj) {
- _cleanup_(barrier_destroy) Barrier b = { };
- int r;
-
- assert_return(obj, -EINVAL);
-
- b.me = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
- if (b.me < 0)
- return -errno;
-
- b.them = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
- if (b.them < 0)
- return -errno;
+int barrier_create(Barrier *b) {
+ assert(b);
- r = pipe2(b.pipe, O_CLOEXEC | O_NONBLOCK);
- if (r < 0)
+ if ((b->me = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) < 0 ||
+ (b->them = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK)) < 0 ||
+ pipe2(b->pipe, O_CLOEXEC | O_NONBLOCK) < 0) {
+ barrier_destroy(b);
return -errno;
+ }
- memcpy(obj, &b, sizeof(b));
- zero(b);
return 0;
}
@@ -138,15 +128,11 @@ int barrier_init(Barrier *obj) {
* barrier_destroy() - Destroy a barrier object
* @b: barrier to destroy or NULL
*
- * This destroys a barrier object that has previously been initialized via
- * barrier_init(). The object is released and reset to invalid state.
- * Therefore, it is safe to call barrier_destroy() multiple times or even if
- * barrier_init() failed. However, you must not call barrier_destroy() if you
- * never called barrier_init() on the object before.
- *
- * It is safe to initialize a barrier via zero() / memset(.., 0, ...). Even
- * though it has embedded FDs, barrier_destroy() can deal with zeroed objects
- * just fine.
+ * This destroys a barrier object that has previously been passed to
+ * barrier_create(). The object is released and reset to invalid
+ * state. Therefore, it is safe to call barrier_destroy() multiple
+ * times or even if barrier_create() failed. However, barrier must be
+ * always initalized with BARRIER_NULL.
*
* If @b is NULL, this is a no-op.
*/
@@ -154,21 +140,9 @@ void barrier_destroy(Barrier *b) {
if (!b)
return;
- /* @me and @them cannot be both FD 0. Lets be pedantic and check the
- * pipes and barriers, too. If all are 0, the object was zero()ed and
- * is invalid. This allows users to use zero(barrier) to reset the
- * backing memory. */
- if (b->me == 0 &&
- b->them == 0 &&
- b->pipe[0] == 0 &&
- b->pipe[1] == 0 &&
- b->barriers == 0)
- return;
-
b->me = safe_close(b->me);
b->them = safe_close(b->them);
- b->pipe[0] = safe_close(b->pipe[0]);
- b->pipe[1] = safe_close(b->pipe[1]);
+ safe_close_pair(b->pipe);
b->barriers = 0;
}
@@ -177,13 +151,14 @@ void barrier_destroy(Barrier *b) {
* @b: barrier to operate on
* @role: role to set on the barrier
*
- * This sets the roles on a barrier object. This is needed to know which
- * side of the barrier you're on. Usually, the parent creates the barrier via
- * barrier_init() and then calls fork() or clone(). Therefore, the FDs are
- * duplicated and the child retains the same barrier object.
+ * This sets the roles on a barrier object. This is needed to know
+ * which side of the barrier you're on. Usually, the parent creates
+ * the barrier via barrier_create() and then calls fork() or clone().
+ * Therefore, the FDs are duplicated and the child retains the same
+ * barrier object.
*
- * Both sides need to call barrier_set_role() after fork() or clone() are done.
- * If this is not done, barriers will not work correctly.
+ * Both sides need to call barrier_set_role() after fork() or clone()
+ * are done. If this is not done, barriers will not work correctly.
*
* Note that barriers could be supported without fork() or clone(). However,
* this is currently not needed so it hasn't been implemented.
@@ -196,9 +171,9 @@ void barrier_set_role(Barrier *b, unsigned int role) {
/* make sure this is only called once */
assert(b->pipe[1] >= 0 && b->pipe[1] >= 0);
- if (role == BARRIER_PARENT) {
+ if (role == BARRIER_PARENT)
b->pipe[1] = safe_close(b->pipe[1]);
- } else {
+ else {
b->pipe[0] = safe_close(b->pipe[0]);
/* swap me/them for children */
@@ -218,7 +193,7 @@ static bool barrier_write(Barrier *b, uint64_t buf) {
do {
len = write(b->me, &buf, sizeof(buf));
- } while (len < 0 && (errno == EAGAIN || errno == EINTR));
+ } while (len < 0 && IN_SET(errno, EAGAIN, EINTR));
if (len != sizeof(buf))
goto error;
@@ -229,9 +204,8 @@ static bool barrier_write(Barrier *b, uint64_t buf) {
b->barriers = BARRIER_WE_ABORTED;
else
b->barriers = BARRIER_I_ABORTED;
- } else if (!barrier_is_aborted(b)) {
+ } else if (!barrier_is_aborted(b))
b->barriers += buf;
- }
return !barrier_i_aborted(b);
@@ -241,52 +215,48 @@ error:
* pipe-ends and treat this as abortion. The other end will notice the
* pipe-close and treat it as abortion, too. */
- b->pipe[0] = safe_close(b->pipe[0]);
- b->pipe[1] = safe_close(b->pipe[1]);
+ safe_close_pair(b->pipe);
b->barriers = BARRIER_WE_ABORTED;
return false;
}
/* waits for barriers; returns false if they aborted, otherwise true */
static bool barrier_read(Barrier *b, int64_t comp) {
- uint64_t buf;
- ssize_t len;
- struct pollfd pfd[2] = { };
- int r;
-
if (barrier_they_aborted(b))
return false;
while (b->barriers > comp) {
- pfd[0].fd = (b->pipe[0] >= 0) ? b->pipe[0] : b->pipe[1];
- pfd[0].events = POLLHUP;
- pfd[0].revents = 0;
- pfd[1].fd = b->them;
- pfd[1].events = POLLIN;
- pfd[1].revents = 0;
+ struct pollfd pfd[2] = {
+ { .fd = b->pipe[0] >= 0 ? b->pipe[0] : b->pipe[1],
+ .events = POLLHUP },
+ { .fd = b->them,
+ .events = POLLIN }};
+ uint64_t buf;
+ int r;
r = poll(pfd, 2, -1);
- if (r < 0 && (errno == EAGAIN || errno == EINTR))
+ if (r < 0 && IN_SET(errno, EAGAIN, EINTR))
continue;
else if (r < 0)
goto error;
if (pfd[1].revents) {
- /* events on @them signal us new data */
+ ssize_t len;
+
+ /* events on @them signal new data for us */
len = read(b->them, &buf, sizeof(buf));
- if (len < 0 && (errno == EAGAIN || errno == EINTR))
+ if (len < 0 && IN_SET(errno, EAGAIN, EINTR))
continue;
if (len != sizeof(buf))
goto error;
- } else if (pfd[0].revents & (POLLHUP | POLLERR | POLLNVAL)) {
+ } else if (pfd[0].revents & (POLLHUP | POLLERR | POLLNVAL))
/* POLLHUP on the pipe tells us the other side exited.
* We treat this as implicit abortion. But we only
* handle it if there's no event on the eventfd. This
* guarantees that exit-abortions do not overwrite real
* barriers. */
buf = BARRIER_ABORTION;
- }
/* lock if they aborted */
if (buf >= (uint64_t)BARRIER_ABORTION) {
@@ -294,9 +264,8 @@ static bool barrier_read(Barrier *b, int64_t comp) {
b->barriers = BARRIER_WE_ABORTED;
else
b->barriers = BARRIER_THEY_ABORTED;
- } else if (!barrier_is_aborted(b)) {
+ } else if (!barrier_is_aborted(b))
b->barriers -= buf;
- }
}
return !barrier_they_aborted(b);
@@ -307,8 +276,7 @@ error:
* pipe-ends and treat this as abortion. The other end will notice the
* pipe-close and treat it as abortion, too. */
- b->pipe[0] = safe_close(b->pipe[0]);
- b->pipe[1] = safe_close(b->pipe[1]);
+ safe_close_pair(b->pipe);
b->barriers = BARRIER_WE_ABORTED;
return false;
}
diff --git a/src/shared/barrier.h b/src/shared/barrier.h
index 7f76ec7910..53b4439fb2 100644
--- a/src/shared/barrier.h
+++ b/src/shared/barrier.h
@@ -57,7 +57,9 @@ struct Barrier {
int64_t barriers;
};
-int barrier_init(Barrier *obj);
+#define BARRIER_NULL {-1, -1, {-1, -1}, 0}
+
+int barrier_create(Barrier *obj);
void barrier_destroy(Barrier *b);
void barrier_set_role(Barrier *b, unsigned int role);
diff --git a/src/shared/pty.c b/src/shared/pty.c
index 11d76f825f..2863da489c 100644
--- a/src/shared/pty.c
+++ b/src/shared/pty.c
@@ -105,6 +105,7 @@ int pty_new(Pty **out) {
pty->ref = 1;
pty->fd = -1;
+ pty->barrier = (Barrier) BARRIER_NULL;
pty->fd = posix_openpt(O_RDWR | O_NOCTTY | O_CLOEXEC | O_NONBLOCK);
if (pty->fd < 0)
@@ -127,7 +128,7 @@ int pty_new(Pty **out) {
if (r < 0)
return -errno;
- r = barrier_init(&pty->barrier);
+ r = barrier_create(&pty->barrier);
if (r < 0)
return r;
diff --git a/src/test/test-barrier.c b/src/test/test-barrier.c
index 640e508679..672a51e2c3 100644
--- a/src/test/test-barrier.c
+++ b/src/test/test-barrier.c
@@ -59,10 +59,10 @@ static void msleep(unsigned long msecs) {
#define TEST_BARRIER(_FUNCTION, _CHILD_CODE, _WAIT_CHILD, _PARENT_CODE, _WAIT_PARENT) \
static void _FUNCTION(void) { \
- Barrier b; \
+ Barrier b = BARRIER_NULL; \
pid_t pid1, pid2; \
\
- assert_se(barrier_init(&b) >= 0); \
+ assert_se(barrier_create(&b) >= 0); \
\
pid1 = fork(); \
assert_se(pid1 >= 0); \