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.

diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c
index 657512d661fcb0932d3a7409cb823225faad587b..7c47f6ecd2434356a3a1f6851fbdeaca2238f5bc 100644 (file)
--- a/src/nspawn/nspawn.c
+++ b/src/nspawn/nspawn.c
@@ -3073,13 +3073,13 @@ int main(int argc, char *argv[]) {
 
         for (;;) {
                 ContainerStatus container_status;
 
         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,
                 };
 
                 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;
                 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 c198329cb0196677e1768fad0b1e90f3e6962660..4ac42d023943cd2d76d17b46f4287c9f582e8a7f 100644 (file)
--- 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
  * @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.
  */
  *
  * 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;
                 return -errno;

+        }

 
 

-        memcpy(obj, &b, sizeof(b));
-        zero(b);

         return 0;
 }
 
         return 0;
 }
 


@@ -138,15 +128,11 @@ int barrier_init(Barrier *obj) {
  * barrier_destroy() - Destroy a barrier object
  * @b: barrier to destroy or NULL
  *
  * 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.
  */
  *
  * If @b is NULL, this is a no-op.
  */


@@ -154,21 +140,9 @@ void barrier_destroy(Barrier *b) {
         if (!b)
                 return;
 
         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->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;
 }
 
         b->barriers = 0;
 }
 


@@ -177,13 +151,14 @@ void barrier_destroy(Barrier *b) {
  * @b: barrier to operate on
  * @role: role to set on the barrier
  *
  * @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.
  *
  * 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);
 
         /* 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]);
                 b->pipe[1] = safe_close(b->pipe[1]);

-        } else {
+        else {

                 b->pipe[0] = safe_close(b->pipe[0]);
 
                 /* swap me/them for children */
                 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));
 
         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;
 
         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;
                         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;
                 b->barriers += buf;

-        }

 
         return !barrier_i_aborted(b);
 
 
         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. */
 
          * 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) {
         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) {
         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);
 
                 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) {
                         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));
                         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;
                                 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;
                         /* 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) {
 
                 /* 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;
                                 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;
                         b->barriers -= buf;

-                }

         }
 
         return !barrier_they_aborted(b);
         }
 
         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. */
 
          * 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;
 }
         b->barriers = BARRIER_WE_ABORTED;
         return false;
 }

diff --git a/src/shared/barrier.h b/src/shared/barrier.h
index 7f76ec7910283dff10edde66d3122839b31e5e66..53b4439fb2b5c78d03960dbc97e32914e463f670 100644 (file)
--- a/src/shared/barrier.h
+++ b/src/shared/barrier.h
@@ -57,7 +57,9 @@ struct Barrier {
         int64_t barriers;
 };
 
         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);
 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 11d76f825fd890d2569ba52924cf035047816424..2863da489cabb140bd6e40b2368d7b5fc4deb87b 100644 (file)
--- 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->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)
 
         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;
 
         if (r < 0)
                 return -errno;
 

-        r = barrier_init(&pty->barrier);
+        r = barrier_create(&pty->barrier);

         if (r < 0)
                 return r;
 
         if (r < 0)
                 return r;
 

diff --git a/src/test/test-barrier.c b/src/test/test-barrier.c
index 640e50867921fc6c396c0a6e2745d26998362140..672a51e2c39f619068a9c2603a18b9afa2615d6a 100644 (file)
--- 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) {                                   \
 
 #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;                                       \
                                                                         \
                 pid_t pid1, pid2;                                       \
                                                                         \

-                assert_se(barrier_init(&b) >= 0);                       \
+                assert_se(barrier_create(&b) >= 0);                     \

                                                                         \
                 pid1 = fork();                                          \
                 assert_se(pid1 >= 0);                                   \
                                                                         \
                 pid1 = fork();                                          \
                 assert_se(pid1 >= 0);                                   \