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

[Denys Vlasenko]

Applied with some changes, thank you

On Mon, Nov 21, 2022 at 2:58 PM Xiaoming Ni <nixiaoming at huawei.com> wrote:
>
> 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 | 58 +++++++++++++++++++++++++---------------------------
>  1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/libbb/loop.c b/libbb/loop.c
> index c517ceb13..6c28e683a 100644
> --- a/libbb/loop.c
> +++ b/libbb/loop.c
> @@ -9,6 +9,7 @@
>   */
>  #include "libbb.h"
>  #include <linux/version.h>
> +#include <assert.h>
>
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
>
> @@ -96,6 +97,22 @@ int FAST_FUNC get_free_loop(void)
>         return loopdevno; /* can be -1 if error */
>  }
>
> +static int get_next_free_loop(char *dev, __attribute__((unused)) size_t dev_size, int id)
> +{
> +       int loopdevno;
> +       assert(dev_size >= LOOP_NAMESIZE);
> +       loopdevno = get_free_loop();
> +       if (loopdevno >= 0) {
> +               sprintf(dev, LOOP_FORMAT, loopdevno);
> +               return 1; /* use /dev/loop-control */
> +       }
> +       if (loopdevno == -2) {
> +               sprintf(dev, LOOP_FORMAT, id);
> +               return 2;
> +       }
> +       return -1; /* no free loop devices */
> +}
> +
>  static int open_file(const char *file, unsigned flags, int *mode)
>  {
>         int ffd;
> @@ -132,30 +149,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, sizeof(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 +201,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 +228,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 +235,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
>