shared/loop-util: fix error handling in loop_device_make_full()

The function no longer returns the fd. This complicated semantics, because it
wasn't clear what holds the ownership: the return value or the output
parameter.  There were no users of the fd in the return value, so let's
simplify things conceptually and only return the fd once.

Reduce the scope of variables.

LOOP_CLR_FD was called on the wrong fd. Let's use a cleanup function to make
this automatic and reduce chances of a mixup in the future.

CID 1408498.

diff --git a/src/shared/loop-util.c b/src/shared/loop-util.c
index 2b17df6b5d042faee663e9185a3a16298a29a762..c2d28a348ce814a706f7b16330cb4d7de0aa917a 100644 (file)
--- a/src/shared/loop-util.c
+++ b/src/shared/loop-util.c
@@ -20,6 +20,13 @@
 #include "stat-util.h"
 #include "stdio-util.h"
 
+static void cleanup_clear_loop_close(int *fd) {
+        if (*fd >= 0) {
+                (void) ioctl(*fd, LOOP_CLR_FD);
+                (void) safe_close(*fd);
+        }
+}
+
 int loop_device_make_full(
                 int fd,
                 int open_flags,
@@ -28,9 +35,7 @@ int loop_device_make_full(
                 uint32_t loop_flags,
                 LoopDevice **ret) {
 
-        _cleanup_close_ int control = -1, loop = -1;
         _cleanup_free_ char *loopdev = NULL;
-        unsigned n_attempts = 0;
         struct loop_info64 info;
         LoopDevice *d = NULL;
         struct stat st;
@@ -69,7 +74,6 @@ int loop_device_make_full(
                         d = new(LoopDevice, 1);
                         if (!d)
                                 return -ENOMEM;
-
                         *d = (LoopDevice) {
                                 .fd = TAKE_FD(copy),
                                 .nr = nr,
@@ -86,13 +90,18 @@ int loop_device_make_full(
                         return r;
         }
 
+        _cleanup_close_ int control = -1;
+        _cleanup_(cleanup_clear_loop_close) int loop_with_fd = -1;
+
         control = open("/dev/loop-control", O_RDWR|O_CLOEXEC|O_NOCTTY|O_NONBLOCK);
         if (control < 0)
                 return -errno;
 
         /* Loop around LOOP_CTL_GET_FREE, since at the moment we attempt to open the returned device it might
          * be gone already, taken by somebody else racing against us. */
-        for (;;) {
+        for (unsigned n_attempts = 0;;) {
+                _cleanup_close_ int loop = -1;
+
                 nr = ioctl(control, LOOP_CTL_GET_FREE);
                 if (nr < 0)
                         return -errno;
@@ -103,17 +112,16 @@ int loop_device_make_full(
                 loop = open(loopdev, O_CLOEXEC|O_NONBLOCK|O_NOCTTY|open_flags);
                 if (loop < 0)
                         return -errno;
-                if (ioctl(loop, LOOP_SET_FD, fd) < 0) {
-                        if (errno != EBUSY)
-                                return -errno;
-
-                        if (++n_attempts >= 64) /* Give up eventually */
-                                return -EBUSY;
-                } else
+                if (ioctl(loop, LOOP_SET_FD, fd) >= 0) {
+                        loop_with_fd = TAKE_FD(loop);
                         break;
+                }
+                if (errno != EBUSY)
+                        return -errno;
+                if (++n_attempts >= 64) /* Give up eventually */
+                        return -EBUSY;
 
                 loopdev = mfree(loopdev);
-                loop = safe_close(loop);
         }
 
         info = (struct loop_info64) {
@@ -123,33 +131,20 @@ int loop_device_make_full(
                 .lo_sizelimit = size == UINT64_MAX ? 0 : size,
         };
 
-        if (ioctl(loop, LOOP_SET_STATUS64, &info) < 0) {
-                r = -errno;
-                goto fail;
-        }
+        if (ioctl(loop_with_fd, LOOP_SET_STATUS64, &info) < 0)
+                return -errno;
 
         d = new(LoopDevice, 1);
-        if (!d) {
-                r = -ENOMEM;
-                goto fail;
-        }
-
+        if (!d)
+                return -ENOMEM;
         *d = (LoopDevice) {
-                .fd = TAKE_FD(loop),
+                .fd = TAKE_FD(loop_with_fd),
                 .node = TAKE_PTR(loopdev),
                 .nr = nr,
         };
 
         *ret = d;
-        return d->fd;
-
-fail:
-        if (fd >= 0)
-                (void) ioctl(fd, LOOP_CLR_FD);
-        if (d && d->fd >= 0)
-                (void) ioctl(d->fd, LOOP_CLR_FD);
-
-        return r;
+        return 0;
 }
 
 int loop_device_make_by_path(const char *path, int open_flags, uint32_t loop_flags, LoopDevice **ret) {