AW: [PATCH 2/9] loop:refactor: extract subfunction get_next_free_loop()

[Walter Harms]

hi,
just a minor comment.

do not use i as name for return value,
most ppl use it as loop counter, triggers the
wrong circuits in the brain. (rule of least surprise).
just name it err or what you like,

and please untangle the if()

if (err>=0)
   return
if (err==-2)
   return

return -1

just my 2 cents ...


________________________________________
Von: busybox <busybox-bounces at busybox.net> im Auftrag von Xiaoming Ni <nixiaoming at huawei.com>
Gesendet: Freitag, 18. November 2022 02:01:51
An: busybox at busybox.net; vda.linux at googlemail.com
Cc: wangle6 at huawei.com
Betreff: [PATCH 2/9] loop:refactor: extract subfunction get_next_free_loop()

Step 2 of micro-refactoring the set_loop function ()
        Extract subfunction get_next_free_loop() from set_loop()

Also fix miss free(try) when stat(try) and mknod fail

function                                             old     new   delta
set_loop                                             758     734     -24
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-24)             Total: -24 bytes

Fixes: 3448914e8cc5 ("mount,losetup: use /dev/loop-control is it exists")
Signed-off-by: Xiaoming Ni <nixiaoming at huawei.com>
---
 libbb/loop.c | 55 ++++++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index c517ceb13..71fd8c1bc 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -96,6 +96,20 @@ int FAST_FUNC get_free_loop(void)
        return loopdevno; /* can be -1 if error */
 }

+static int get_next_free_loop(char *dev, int id)
+{
+       int i = get_free_loop();
+       if (i >= 0) {
+               sprintf(dev, LOOP_FORMAT, i);
+               return 1; /* use /dev/loop-control */
+       } else if (i == -2) {
+               sprintf(dev, LOOP_FORMAT, id);
+               return 2;
+       } else {
+               return -1; /* no free loop devices */
+       }
+}
+
 static int open_file(const char *file, unsigned flags, int *mode)
 {
        int ffd;
@@ -132,30 +146,26 @@ int FAST_FUNC set_loop(char **device, const char *file, unsigned long long offse

        try = *device;
        if (!try) {
- get_free_loopN:
-               i = get_free_loop();
-               if (i == -1) {
-                       close(ffd);
-                       return -1; /* no free loop devices */
-               }
-               if (i >= 0) {
-                       try = xasprintf(LOOP_FORMAT, i);
-                       goto open_lfd;
-               }
-               /* i == -2: no /dev/loop-control. Do an old-style search for a free device */
                try = dev;
        }

        /* Find a loop device */
        /* 0xfffff is a max possible minor number in Linux circa 2010 */
        for (i = 0; i <= 0xfffff; i++) {
-               sprintf(dev, LOOP_FORMAT, i);
+               if (!*device) {
+                       rc = get_next_free_loop(dev, i);
+                       if (rc == -1) {
+                               break; /* no free loop devices */
+                       } else if (rc == 1) {
+                               goto open_lfd;
+                       }
+               }

                IF_FEATURE_MOUNT_LOOP_CREATE(errno = 0;)
                if (stat(try, &statbuf) != 0 || !S_ISBLK(statbuf.st_mode)) {
                        if (ENABLE_FEATURE_MOUNT_LOOP_CREATE
                         && errno == ENOENT
-                        && try == dev
+                        && (!*device)
                        ) {
                                /* Node doesn't exist, try to create it */
                                if (mknod(dev, S_IFBLK|0644, makedev(7, i)) == 0)
@@ -188,13 +198,10 @@ int FAST_FUNC set_loop(char **device, const char *file, unsigned long long offse
                        /* Associate free loop device with file */
                        if (ioctl(lfd, LOOP_SET_FD, ffd)) {
                                /* Ouch. Are we racing with other mount? */
-                               if (!*device   /* yes */
-                                && try != dev /* tried a _kernel-offered_ loopN? */
-                               ) {
-                                       free(try);
+                               if (!*device) {
                                        close(lfd);
 //TODO: add "if (--failcount != 0) ..."?
-                                       goto get_free_loopN;
+                                       continue;
                                }
                                goto close_and_try_next_loopN;
                        }
@@ -218,8 +225,6 @@ int FAST_FUNC set_loop(char **device, const char *file, unsigned long long offse
                        }
                        if (rc == 0) {
                                /* SUCCESS! */
-                               if (try != dev) /* tried a kernel-offered free loopN? */
-                                       *device = try; /* malloced */
                                if (!*device)   /* was looping in search of free "/dev/loopN"? */
                                        *device = xstrdup(dev);
                                rc = lfd; /* return this */
@@ -227,16 +232,6 @@ int FAST_FUNC set_loop(char **device, const char *file, unsigned long long offse
                        }
                        /* failure, undo LOOP_SET_FD */
                        ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
-               } else {
-                       /* device is not free (rc == 0), or error other than ENXIO */
-                       if (rc == 0     /* device is not free? */
-                        && !*device    /* racing with other mount? */
-                        && try != dev  /* tried a _kernel-offered_ loopN? */
-                       ) {
-                               free(try);
-                               close(lfd);
-                               goto get_free_loopN;
-                       }
                }
  close_and_try_next_loopN:
                close(lfd);
--
2.27.0

_______________________________________________
busybox mailing list
busybox at busybox.net
http://lists.busybox.net/mailman/listinfo/busybox