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

[Kang-Che Sung]

On Mon, Nov 21, 2022 at 9:19 AM Xiaoming Ni  wrote:
> static int get_next_free_loop(char *dev, size_t dev_size, int id)
> {
>  int loopdevno = get_free_loop();
>  if (loopdevno >= 0) {
>  snprintf(dev, dev_size, LOOP_FORMAT, loopdevno);
>  return 1; /* use /dev/loop-control */
>  }
>  if (loopdevno == -2) {
>  snprintf(dev, dev_size, LOOP_FORMAT, id);
>  return 2;
>  }
>  return -1; /* no free loop devices */
> }
>
> If the dev_size parameter is added to get_next_free_loop(), the code
> size increases, Is it worth?
>
> function old new   delta
> set_loop 734 744 +10
> --
> (add/remove: 0/0 grow/shrink: 1/0 up/down: 10/0)   Total: 10
> bytes

No, that isn't what I mean. sprintf() is faster than snprintf() when
we are sure the string buffer would never overflow.
Just keep using sprintf() here but add a statement before it:
`assert(dev_size >= LOOP_NAMESIZE);`
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH v2 6/9] loop:refactor: Use a structure to reduce parameters

[Xiaoming Ni]

On 2022/11/21 12:28, Kang-Che Sung wrote:

On Mon, Nov 21, 2022 at 9:31 AM Xiaoming Ni  wrote:

Also, it is unclear why there is the need to clone the loopinfo buffer.

  > /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
  > /* (this code path is not tested) */
  > -   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
  > -   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
  > +   loopinfo2.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
  > +   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
  > }
  > if (rc == 0) {
  > return lfd;

...



Pardon for my ignorance, but does the LOOP_SET_STATUS64 ioctl modify
the `loopinfo` object internally?


in linux kernel, drivers/block/loop.c:
static int
loop_set_status(struct loop_device *lo, const struct loop_info64 *info)

static int
loop_set_status64(struct loop_device *lo, const struct loop_info64 
__user *arg)



If the answer is yes, then it might not be a good idea to pass the
`loopinfo` structure to set_loop_configure().
I think it might be better to create the object on the fly (i.e. drop
this patch).
Otherwise, let set_loop_configure pass in a `const bb_loop_info *`
object, when we are sure it would never be modified internally.

thanks
 I'll add const modifiers in the next version.

Thanks

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH v2 6/9] loop:refactor: Use a structure to reduce parameters

[Kang-Che Sung]

On Mon, Nov 21, 2022 at 9:31 AM Xiaoming Ni  wrote:
> > Also, it is unclear why there is the need to clone the loopinfo buffer.
> >
> >  > /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
> >  > /* (this code path is not tested) */
> >  > -   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
> >  > -   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
> >  > +   loopinfo2.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
> >  > +   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
> >  > }
> >  > if (rc == 0) {
> >  > return lfd;
> ...
>

Pardon for my ignorance, but does the LOOP_SET_STATUS64 ioctl modify
the `loopinfo` object internally?
If the answer is yes, then it might not be a good idea to pass the
`loopinfo` structure to set_loop_configure().
I think it might be better to create the object on the fly (i.e. drop
this patch).
Otherwise, let set_loop_configure pass in a `const bb_loop_info *`
object, when we are sure it would never be modified internally.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 6/9] loop:refactor: Use a structure to reduce parameters

[Xiaoming Ni]

Step 6 of micro-refactoring the set_loop():
Use structs to avoid transferring a large number of parameters
in set_loop_configure() and set_loop_info()

function old new   delta
set_loop 708 720 +12
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 12/0)   Total: 12 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 54 +---
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index dc8ecc9a8..0b1336a02 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -126,32 +126,21 @@ static int open_file(const char *file, unsigned flags, 
int *mode)
return ffd;
 }
 
-static int set_loop_configure(int ffd, int lfd, const char *file,
-   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+static int set_loop_configure(int ffd, int lfd, bb_loop_info *loopinfo)
 {
int rc;
-   bb_loop_info loopinfo;
+   bb_loop_info loopinfo2;
/* Associate free loop device with file */
if (ioctl(lfd, LOOP_SET_FD, ffd)) {
return -1;
}
-   memset(, 0, sizeof(loopinfo));
-   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
-   loopinfo.lo_offset = offset;
-   loopinfo.lo_sizelimit = sizelimit;
-   /*
-* Used by mount to set LO_FLAGS_AUTOCLEAR.
-* LO_FLAGS_READ_ONLY is not set because RO is controlled by open type 
of the file.
-* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
-* is wrong (would free the loop device!)
-*/
-   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
-   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, loopinfo);
+   if (rc != 0 && (loopinfo->lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   memcpy(, loopinfo, sizeof(*loopinfo));
/* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
/* (this code path is not tested) */
-   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   loopinfo2.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
}
if (rc == 0) {
return lfd;
@@ -161,21 +150,36 @@ static int set_loop_configure(int ffd, int lfd, const 
char *file,
return -1;
 }
 
-static int set_loop_info(int ffd, int lfd, const char *file,
-   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+static int set_loop_info(int ffd, int lfd, bb_loop_info *loopinfo)
 {
int rc;
-   bb_loop_info loopinfo;
+   bb_loop_info tmp;
 
-   rc = ioctl(lfd, BB_LOOP_GET_STATUS, );
+   rc = ioctl(lfd, BB_LOOP_GET_STATUS, );
 
/* If device is free, try to claim it */
if (rc && errno == ENXIO) {
-   return set_loop_configure(ffd, lfd, file, offset, sizelimit, 
flags);
+   return set_loop_configure(ffd, lfd, loopinfo);
}
return -1;
 }
 
+static void init_bb_loop_info(bb_loop_info *loopinfo, const char *file,
+   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+{
+   memset(loopinfo, 0, sizeof(*loopinfo));
+   safe_strncpy((char *)loopinfo->lo_file_name, file, LO_NAME_SIZE);
+   loopinfo->lo_offset = offset;
+   loopinfo->lo_sizelimit = sizelimit;
+   /*
+* Used by mount to set LO_FLAGS_AUTOCLEAR.
+* LO_FLAGS_READ_ONLY is not set because RO is controlled by open type 
of the file.
+* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
+* is wrong (would free the loop device!)
+*/
+   loopinfo->lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
+}
+
 /* Returns opened fd to the loop device, <0 on error.
  * *device is loop device to use, or if *device==NULL finds a loop device to
  * mount it on and sets *device to a strdup of that loop device name.
@@ -187,12 +191,14 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
char *try;
struct stat statbuf;
int i, lfd, ffd, mode, rc;
+   bb_loop_info loopinfo;
 
ffd = open_file(file, flags, );
if (ffd < 0) {
return -errno;
}
 
+   init_bb_loop_info(, file, offset, sizelimit, flags);
try = *device;
if (!try) {
try = dev;
@@ -239,7 +245,7 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
}
goto try_next_loopN;
}
-   rc = set_loop_info(ffd, lfd, file, offset, sizelimit, flags);
+   

[PATCH v3 8/9] loop:refactor: extract subfunction set_loop_dev()

[Xiaoming Ni]

Step 8 of micro-refactoring the set_loop():
Extract subfunction open_file() from set_loop_dev()

function old new   delta
set_loop 700 716 +16
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 16/0)   Total: 16 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 39 ---
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 8b207b929..799936765 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -200,6 +200,26 @@ static int do_stat_and_mknod(const char *dev, const char 
*device, int i)
return -1;
 }
 
+static int set_loop_dev(const char *dev, int *mode, int ffd, bb_loop_info 
*loopinfo)
+{
+   int rc;
+   /* Open the sucker and check its loopiness */
+   int lfd = open(dev, *mode);
+   if (lfd < 0 && errno == EROFS) {
+   *mode = O_RDONLY;
+   lfd = open(dev, *mode);
+   }
+   if (lfd < 0) {
+   return lfd;
+   }
+   rc = set_loop_info(ffd, lfd, loopinfo);
+   if (rc == lfd) {
+   return lfd;
+   }
+   close(lfd);
+   return -1;
+}
+
 
 /* Returns opened fd to the loop device, <0 on error.
  * *device is loop device to use, or if *device==NULL finds a loop device to
@@ -210,7 +230,7 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
 {
char dev[LOOP_NAMESIZE];
char *try;
-   int i, lfd, ffd, mode, rc;
+   int i, ffd, mode, rc;
bb_loop_info loopinfo;
 
ffd = open_file(file, flags, );
@@ -241,30 +261,19 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
break;
}
  open_lfd:
-   /* Open the sucker and check its loopiness */
-   lfd = rc = open(try, mode);
-   if (lfd < 0 && errno == EROFS) {
-   mode = O_RDONLY;
-   lfd = rc = open(try, mode);
-   }
-   if (lfd < 0) {
+   rc = set_loop_dev(try, , ffd, );
+   if (rc == -1) {
if (errno == ENXIO) {
/* Happens if loop module is not loaded */
/* rc is -1; */
break;
}
-   goto try_next_loopN;
-   }
-   rc = set_loop_info(ffd, lfd, );
-   if (rc == lfd) {
+   } else {
/* SUCCESS! */
if (!*device)
*device = xstrdup(try);
break;
}
-   close(lfd);
- try_next_loopN:
-   rc = -1;
if (*device) /* was looking for a particular "/dev/loopN"? */
break; /* yes, do not try other names */
} /* for() */
-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 9/9] loop: Add LOOP_CONFIGURE ioctl

[Xiaoming Ni]

LOOP_CONFIGURE is added to Linux 5.8

This allows userspace to completely setup a loop device with a single
ioctl, removing the in-between state where the device can be partially
configured - eg the loop device has a backing file associated with it,
but is reading from the wrong offset.

https://lwn.net/Articles/820408/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3448914e8cc5

Builders can use config to choose which algorithm to build into their busybox 
binary
kernel version >= 5.8, choice CONFIG_LOOP_CONFIGURE
function old new   delta
set_loop 716 639 -77

--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-77) Total: 
-77 bytes
kernel version < 5.8, choice CONFIG_NO_LOOP_CONFIGURE
function old new   delta

--
(add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0) Total: 
0 bytes
kernel version is unknown, choice CONFIG_TRY_LOOP_CONFIGURE
function old new   delta
set_loop 716 832+116

--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 116/0) Total: 
116 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/Config.src | 22 ++
 libbb/loop.c | 47 ++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/libbb/Config.src b/libbb/Config.src
index 66a3ffa23..b7f9ddab4 100644
--- a/libbb/Config.src
+++ b/libbb/Config.src
@@ -369,3 +369,25 @@ config UNICODE_PRESERVE_BROKEN
For example, this means that entering 'l', 's', ' ', 0xff, [Enter]
at shell prompt will list file named 0xff (single char name
with char value 255), not file named '?'.
+
+choice
+   prompt "LOOP_CONFIGURE or LOOP_SET_FD + LOOP_SET_STATUS"
+   default TRY_LOOP_CONFIGURE
+   help
+   LOOP_CONFIGURE is added to Linux 5.8
+   https://lwn.net/Articles/820408/
+   This allows userspace to completely setup a loop device with a single
+   ioctl, removing the in-between state where the device can be partially
+   configured - eg the loop device has a backing file associated with it,
+   but is reading from the wrong offset.
+
+config LOOP_CONFIGURE
+   bool "always uses LOOP_CONFIGURE, kernel version >= 5.8"
+
+config NO_LOOP_CONFIGURE
+   bool "never uses LOOP_CONFIGURE, kernel version < 5.8"
+
+config TRY_LOOP_CONFIGURE
+   bool "try LOOP_CONFIGURE, kernel version is unknown"
+
+endchoice
diff --git a/libbb/loop.c b/libbb/loop.c
index 799936765..14919b318 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -126,7 +126,8 @@ static int open_file(const char *file, unsigned flags, int 
*mode)
return ffd;
 }
 
-static int set_loop_configure(int ffd, int lfd, bb_loop_info *loopinfo)
+#ifndef CONFIG_LOOP_CONFIGURE
+static int set_loop_fd_and_status(int ffd, int lfd, bb_loop_info *loopinfo)
 {
int rc;
bb_loop_info loopinfo2;
@@ -149,6 +150,46 @@ static int set_loop_configure(int ffd, int lfd, 
bb_loop_info *loopinfo)
ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
return -1;
 }
+#endif
+
+#ifndef CONFIG_NO_LOOP_CONFIGURE
+
+#ifndef LOOP_CONFIGURE
+#define LOOP_CONFIGURE 0x4C0A
+struct loop_config {
+   uint32_t fd;
+   uint32_t block_size;
+   struct loop_info64 info;
+   uint64_t __reserved[8];
+};
+#endif
+
+/*
+ * linux v5.8.0
+ * loop: Add LOOP_CONFIGURE ioctl
+ * https://lwn.net/Articles/820408/
+ * 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3448914e8cc5
+ */
+static int set_loop_configure(int ffd, int lfd, bb_loop_info *loopinfo)
+{
+   int rc;
+   struct loop_config config;
+
+   memset(, 0, sizeof(config));
+   config.fd = ffd;
+   memcpy(, loopinfo, sizeof(config.info));
+
+   rc = ioctl(lfd, LOOP_CONFIGURE, );
+   if (rc == 0) {
+   return lfd;
+   }
+#ifdef CONFIG_TRY_LOOP_CONFIGURE
+   if (rc == -1 && errno == EINVAL) /* The system may not support 
LOOP_CONFIGURE. */
+   return set_loop_fd_and_status(ffd, lfd, loopinfo);
+#endif
+   return -1;
+}
+#endif
 
 static int set_loop_info(int ffd, int lfd, bb_loop_info *loopinfo)
 {
@@ -159,7 +200,11 @@ static int set_loop_info(int ffd, int lfd, bb_loop_info 
*loopinfo)
 
/* If device is free, try to claim it */
if (rc && errno == ENXIO) {
+#ifdef CONFIG_NO_LOOP_CONFIGURE
+   return set_loop_fd_and_status(ffd, lfd, loopinfo);
+#else
return 

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

[Xiaoming Ni]

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 
---
 libbb/loop.c | 55 
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index c517ceb13..baa02dabb 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 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 +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 */
/* 0xf is a max possible minor number in Linux circa 2010 */
for (i = 0; i <= 0xf; 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, ) != 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 

[PATCH v3 4/9] loop:refactor: extract subfunction set_loop_info()

[Xiaoming Ni]

Step 4 of micro-refactoring the set_loop():
Extract subfunction set_loop_info() from set_loop()

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

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 91 +++-
 1 file changed, 47 insertions(+), 44 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 147597ab9..34e2dce7a 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -126,6 +126,47 @@ static int open_file(const char *file, unsigned flags, int 
*mode)
return ffd;
 }
 
+static int set_loop_info(int ffd, int lfd, const char *file,
+   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+{
+   int rc;
+   bb_loop_info loopinfo;
+
+   rc = ioctl(lfd, BB_LOOP_GET_STATUS, );
+
+   /* If device is free, try to claim it */
+   if (rc && errno == ENXIO) {
+   /* Associate free loop device with file */
+   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
+   return -1;
+   }
+   memset(, 0, sizeof(loopinfo));
+   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
+   loopinfo.lo_offset = offset;
+   loopinfo.lo_sizelimit = sizelimit;
+   /*
+* Used by mount to set LO_FLAGS_AUTOCLEAR.
+* LO_FLAGS_READ_ONLY is not set because RO is controlled by 
open type of the file.
+* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
+* is wrong (would free the loop device!)
+*/
+   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
+   /* (this code path is not tested) */
+   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   }
+   if (rc == 0) {
+   return lfd;
+   }
+   /* failure, undo LOOP_SET_FD */
+   ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
+   }
+   return -1;
+}
+
 /* Returns opened fd to the loop device, <0 on error.
  * *device is loop device to use, or if *device==NULL finds a loop device to
  * mount it on and sets *device to a strdup of that loop device name.
@@ -135,7 +176,6 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
 {
char dev[LOOP_NAMESIZE];
char *try;
-   bb_loop_info loopinfo;
struct stat statbuf;
int i, lfd, ffd, mode, rc;
 
@@ -190,49 +230,12 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
}
goto try_next_loopN;
}
-
-   rc = ioctl(lfd, BB_LOOP_GET_STATUS, );
-
-   /* If device is free, try to claim it */
-   if (rc && errno == ENXIO) {
-   /* Associate free loop device with file */
-   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
-   close(lfd);
-   /* Ouch. Are we racing with other mount? */
-   if (!*device) {
-//TODO: add "if (--failcount != 0) ..."?
-   continue;
-   } else {
-   break;
-   }
-   }
-   memset(, 0, sizeof(loopinfo));
-   safe_strncpy((char *)loopinfo.lo_file_name, file, 
LO_NAME_SIZE);
-   loopinfo.lo_offset = offset;
-   loopinfo.lo_sizelimit = sizelimit;
-   /*
-* Used by mount to set LO_FLAGS_AUTOCLEAR.
-* LO_FLAGS_READ_ONLY is not set because RO is 
controlled by open type of the file.
-* Note that closing LO_FLAGS_AUTOCLEARed lfd before 
mount
-* is wrong (would free the loop device!)
-*/
-   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
-   if (rc != 0 && (loopinfo.lo_flags & 
BB_LO_FLAGS_AUTOCLEAR)) {
-   /* Old kernel, does not support 
LO_FLAGS_AUTOCLEAR? */
-   /* (this code path is not tested) */
-   

[PATCH v3 5/9] loop:refactor: extract subfunction set_loop_configure()

[Xiaoming Ni]

Step 5 of micro-refactoring the set_loop():
Extract subfunction set_loop_configure()

function old new   delta
set_loop 700 708  +8
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 8/0) Total: 8 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 63 ++--
 1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 34e2dce7a..dc8ecc9a8 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -126,6 +126,41 @@ static int open_file(const char *file, unsigned flags, int 
*mode)
return ffd;
 }
 
+static int set_loop_configure(int ffd, int lfd, const char *file,
+   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+{
+   int rc;
+   bb_loop_info loopinfo;
+   /* Associate free loop device with file */
+   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
+   return -1;
+   }
+   memset(, 0, sizeof(loopinfo));
+   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
+   loopinfo.lo_offset = offset;
+   loopinfo.lo_sizelimit = sizelimit;
+   /*
+* Used by mount to set LO_FLAGS_AUTOCLEAR.
+* LO_FLAGS_READ_ONLY is not set because RO is controlled by open type 
of the file.
+* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
+* is wrong (would free the loop device!)
+*/
+   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
+   /* (this code path is not tested) */
+   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   }
+   if (rc == 0) {
+   return lfd;
+   }
+   /* failure, undo LOOP_SET_FD */
+   ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
+   return -1;
+}
+
 static int set_loop_info(int ffd, int lfd, const char *file,
unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
 {
@@ -136,33 +171,7 @@ static int set_loop_info(int ffd, int lfd, const char 
*file,
 
/* If device is free, try to claim it */
if (rc && errno == ENXIO) {
-   /* Associate free loop device with file */
-   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
-   return -1;
-   }
-   memset(, 0, sizeof(loopinfo));
-   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
-   loopinfo.lo_offset = offset;
-   loopinfo.lo_sizelimit = sizelimit;
-   /*
-* Used by mount to set LO_FLAGS_AUTOCLEAR.
-* LO_FLAGS_READ_ONLY is not set because RO is controlled by 
open type of the file.
-* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
-* is wrong (would free the loop device!)
-*/
-   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
-   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
-   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
-   /* (this code path is not tested) */
-   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
-   }
-   if (rc == 0) {
-   return lfd;
-   }
-   /* failure, undo LOOP_SET_FD */
-   ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
+   return set_loop_configure(ffd, lfd, file, offset, sizelimit, 
flags);
}
return -1;
 }
-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 7/9] loop:refactor: Extract subfunction do_stat_and_mknod()

[Xiaoming Ni]

Step 7 of micro-refactoring the set_loop():
Extract subfunction do_stat_and_mknod()

function old new   delta
set_loop 720 700 -20
--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-20) Total: -20 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 0b1336a02..8b207b929 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -180,6 +180,27 @@ static void init_bb_loop_info(bb_loop_info *loopinfo, 
const char *file,
loopinfo->lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
 }
 
+static int do_stat_and_mknod(const char *dev, const char *device, int i)
+{
+   struct stat statbuf;
+
+   IF_FEATURE_MOUNT_LOOP_CREATE(errno = 0;)
+   if (stat(dev, ) == 0 && S_ISBLK(statbuf.st_mode))
+   return 0;
+   if (ENABLE_FEATURE_MOUNT_LOOP_CREATE
+   && errno == ENOENT
+   && (!device)
+  ) {
+   /* Node doesn't exist, try to create it */
+   if (mknod(dev, S_IFBLK|0644, makedev(7, i)) == 0) {
+   return 0;
+   }
+   }
+   /* Ran out of block devices, return failure */
+   return -1;
+}
+
+
 /* Returns opened fd to the loop device, <0 on error.
  * *device is loop device to use, or if *device==NULL finds a loop device to
  * mount it on and sets *device to a strdup of that loop device name.
@@ -189,7 +210,6 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
 {
char dev[LOOP_NAMESIZE];
char *try;
-   struct stat statbuf;
int i, lfd, ffd, mode, rc;
bb_loop_info loopinfo;
 
@@ -216,18 +236,8 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
}
}
 
-   IF_FEATURE_MOUNT_LOOP_CREATE(errno = 0;)
-   if (stat(try, ) != 0 || !S_ISBLK(statbuf.st_mode)) {
-   if (ENABLE_FEATURE_MOUNT_LOOP_CREATE
-&& errno == ENOENT
-&& (!*device)
-   ) {
-   /* Node doesn't exist, try to create it */
-   if (mknod(dev, S_IFBLK|0644, makedev(7, i)) == 
0)
-   goto open_lfd;
-   }
-   /* Ran out of block devices, return failure */
-   rc = -1;
+   rc = do_stat_and_mknod(try, *device, i);
+   if (rc == -1) {
break;
}
  open_lfd:
@@ -249,7 +259,7 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
if (rc == lfd) {
/* SUCCESS! */
if (!*device)
-   *device = xstrdup(dev);
+   *device = xstrdup(try);
break;
}
close(lfd);
-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 3/9] loop:refactor: del close_and_try_next_loopN

[Xiaoming Ni]

Step 3 of micro-refactoring the set_loop() function:
Delete close_and_try_next_loopN.

(add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0) Total: 0 bytes

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index baa02dabb..147597ab9 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -197,13 +197,14 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
if (rc && errno == ENXIO) {
/* Associate free loop device with file */
if (ioctl(lfd, LOOP_SET_FD, ffd)) {
+   close(lfd);
/* Ouch. Are we racing with other mount? */
if (!*device) {
-   close(lfd);
 //TODO: add "if (--failcount != 0) ..."?
continue;
+   } else {
+   break;
}
-   goto close_and_try_next_loopN;
}
memset(, 0, sizeof(loopinfo));
safe_strncpy((char *)loopinfo.lo_file_name, file, 
LO_NAME_SIZE);
@@ -233,7 +234,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
}
- close_and_try_next_loopN:
close(lfd);
  try_next_loopN:
rc = -1;
-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 1/9] loop:refactor: extract subfunction open_file()

[Xiaoming Ni]

Step 1 of micro-refactoring the set_loop():
Extract subfunction open_file() from set_loop()

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

Signed-off-by: Xiaoming Ni 
---
 libbb/loop.c | 25 +
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 750642ade..c517ceb13 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -96,6 +96,22 @@ int FAST_FUNC get_free_loop(void)
return loopdevno; /* can be -1 if error */
 }
 
+static int open_file(const char *file, unsigned flags, int *mode)
+{
+   int ffd;
+   /* open the file.  barf if this doesn't work.  */
+   *mode = (flags & BB_LO_FLAGS_READ_ONLY) ? O_RDONLY : O_RDWR;
+ retry_open_ffd:
+   ffd = open(file, *mode);
+   if (ffd < 0) {
+   if (*mode != O_RDONLY) {
+   *mode = O_RDONLY;
+   goto retry_open_ffd;
+   }
+   }
+   return ffd;
+}
+
 /* Returns opened fd to the loop device, <0 on error.
  * *device is loop device to use, or if *device==NULL finds a loop device to
  * mount it on and sets *device to a strdup of that loop device name.
@@ -109,15 +125,8 @@ int FAST_FUNC set_loop(char **device, const char *file, 
unsigned long long offse
struct stat statbuf;
int i, lfd, ffd, mode, rc;
 
-   /* Open the file.  Barf if this doesn't work.  */
-   mode = (flags & BB_LO_FLAGS_READ_ONLY) ? O_RDONLY : O_RDWR;
- open_ffd:
-   ffd = open(file, mode);
+   ffd = open_file(file, flags, );
if (ffd < 0) {
-   if (mode != O_RDONLY) {
-   mode = O_RDONLY;
-   goto open_ffd;
-   }
return -errno;
}
 
-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH v3 0/9] loop: Micro-refactoring set_loop() and add LOOP_CONFIGURE

[Xiaoming Ni]

LOOP_CONFIGURE is added to Linux 5.8

This allows userspace to completely setup a loop device with a single
ioctl, removing the in-between state where the device can be partially
configured - eg the loop device has a backing file associated with it,
but is reading from the wrong offset.

https://lwn.net/Articles/820408/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3448914e8cc5

patch 1-8: To facilitate the addition of new functions, the code of set_loop() 
is micro-refactored.
function old new   delta
set_loop 760 716 -44

--
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-44) Total: 
-44 bytes

patch 9: add LOOP_CONFIGURE 
Builders can use config to choose which algorithm to build into their 
busybox binary

---
v3: Fine-tuned based on review comments
  patch2: Rename local variables, use "loopdevno" instead of "i" in 
get_next_free_loop().
  patch7: Reduce indentation using guard statements in do_stat_and_mknod().
  patch9: Rename set_loop_configure_old() to set_loop_fd_and_status()
v2: http://lists.busybox.net/pipermail/busybox/2022-November/089990.html 
  changes in patch9:  add config for builder to select
v1: http://lists.busybox.net/pipermail/busybox/2022-November/089969.html
---
*** BLURB HERE ***

Xiaoming Ni (9):
  loop:refactor: extract subfunction open_file()
  loop:refactor: extract subfunction get_next_free_loop()
  loop:refactor: del close_and_try_next_loopN
  loop:refactor: extract subfunction set_loop_info()
  loop:refactor: extract subfunction set_loop_configure()
  loop:refactor: Use a structure to reduce parameters
  loop:refactor: Extract subfunction do_stat_and_mknod()
  loop:refactor: extract subfunction set_loop_dev()
  loop: Add LOOP_CONFIGURE ioctl

 libbb/Config.src |  22 
 libbb/loop.c | 292 ++-
 2 files changed, 211 insertions(+), 103 deletions(-)

-- 
2.27.0

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: AW: [PATCH v2 5/9] loop:refactor: extract subfunction set_loop_configure()

[Xiaoming Ni]

On 2022/11/18 20:36, Walter Harms wrote:

A little tweak make it more readable (i hope)
putting if (rc) direkt behind

   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
   if (rc == 0)
   return lfd;

// no need to check rc again
if ( (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
// try again without   BB_LO_FLAGS_AUTOCLEAR
// does it realy work that way ? no ~ ?
loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
}
  if (rc == 0)
  return lfd;

+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
+   /* (this code path is not tested) */
+   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   }
+   if (rc == 0) {
+   return lfd;
+   }


In this case, duplicate "if(rc==0)" occurs.
Is that worse?

Thanks
Xiaoming Ni




Von: busybox  im Auftrag von Xiaoming Ni 

Gesendet: Freitag, 18. November 2022 13:14:44
An: busybox@busybox.net; vda.li...@googlemail.com; c...@gmx.com; 
explore...@gmail.com
Cc: wang...@huawei.com
Betreff: [PATCH v2 5/9] loop:refactor: extract subfunction set_loop_configure()

Step 5 of micro-refactoring the set_loop():
 Extract subfunction set_loop_configure()

function old new   delta
set_loop 700 708  +8
--
(add/remove: 0/0 grow/shrink: 1/0 up/down: 8/0) Total: 8 bytes

Signed-off-by: Xiaoming Ni 
---
  libbb/loop.c | 63 ++--
  1 file changed, 36 insertions(+), 27 deletions(-)

diff --git a/libbb/loop.c b/libbb/loop.c
index 89adc4f94..914af57b2 100644
--- a/libbb/loop.c
+++ b/libbb/loop.c
@@ -126,6 +126,41 @@ static int open_file(const char *file, unsigned flags, int 
*mode)
 return ffd;
  }

+static int set_loop_configure(int ffd, int lfd, const char *file,
+   unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
+{
+   int rc;
+   bb_loop_info loopinfo;
+   /* Associate free loop device with file */
+   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
+   return -1;
+   }
+   memset(, 0, sizeof(loopinfo));
+   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
+   loopinfo.lo_offset = offset;
+   loopinfo.lo_sizelimit = sizelimit;
+   /*
+* Used by mount to set LO_FLAGS_AUTOCLEAR.
+* LO_FLAGS_READ_ONLY is not set because RO is controlled by open type 
of the file.
+* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
+* is wrong (would free the loop device!)
+*/
+   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
+   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
+   /* (this code path is not tested) */
+   loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
+   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
+   }
+   if (rc == 0) {
+   return lfd;
+   }
+   /* failure, undo LOOP_SET_FD */
+   ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
+   return -1;
+}
+
  static int set_loop_info(int ffd, int lfd, const char *file,
 unsigned long long offset, unsigned long long sizelimit, 
unsigned flags)
  {
@@ -136,33 +171,7 @@ static int set_loop_info(int ffd, int lfd, const char 
*file,

 /* If device is free, try to claim it */
 if (rc && errno == ENXIO) {
-   /* Associate free loop device with file */
-   if (ioctl(lfd, LOOP_SET_FD, ffd)) {
-   return -1;
-   }
-   memset(, 0, sizeof(loopinfo));
-   safe_strncpy((char *)loopinfo.lo_file_name, file, LO_NAME_SIZE);
-   loopinfo.lo_offset = offset;
-   loopinfo.lo_sizelimit = sizelimit;
-   /*
-* Used by mount to set LO_FLAGS_AUTOCLEAR.
-* LO_FLAGS_READ_ONLY is not set because RO is controlled by 
open type of the file.
-* Note that closing LO_FLAGS_AUTOCLEARed lfd before mount
-* is wrong (would free the loop device!)
-*/
-   loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
-   rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
-   if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
-   /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
-   /* (this code path is not tested) */
-   loopinfo.lo_flags -= 

Re: [PATCH v2 9/9] loop: Add LOOP_CONFIGURE ioctl

[Xiaoming Ni]

On 2022/11/19 2:22, Kang-Che Sung wrote:



On Friday, November 18, 2022, Xiaoming Ni > wrote:

 > LOOP_CONFIGURE is added to Linux 5.8
 >
 > This allows userspace to completely setup a loop device with a single
 > ioctl, removing the in-between state where the device can be partially
 > configured - eg the loop device has a backing file associated with it,
 > but is reading from the wrong offset.
 >
 > https://lwn.net/Articles/820408/ 
 > 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3448914e8cc5 


 >
 > kernel version >= 5.8, choice CONFIG_LOOP_CONFIGURE
 >         function                                             old
  new   delta
 >         set_loop                                             716
  832    +116
 > 
--
 >         (add/remove: 0/0 grow/shrink: 1/0 up/down: 116/0)
  Total: 116 bytes

 > kernel version < 5.8, choice CONFIG_NO_LOOP_CONFIGURE
 >         function                                             old
  new   delta
 >         set_loop                                             760
  716     -44
 > 
--
 >         (add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-44)
  Total: -44 bytes

 > kernel version is unknown, choice CONFIG_TRY_LOOP_CONFIGURE
 >         function                                             old
  new   delta
 > 
--
 >         (add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0)  
    Total: 0 bytes

 >
 > Signed-off-by: Xiaoming Ni >

 > ---
 >  libbb/Config.src | 22 ++
 >  libbb/loop.c     | 47 ++-
 >  2 files changed, 68 insertions(+), 1 deletion(-)
 >
 > diff --git a/libbb/Config.src b/libbb/Config.src
 > index 66a3ffa23..b7f9ddab4 100644
 > --- a/libbb/Config.src
 > +++ b/libbb/Config.src
 > @@ -369,3 +369,25 @@ config UNICODE_PRESERVE_BROKEN
 >         For example, this means that entering 'l', 's', ' ', 0xff, 
[Enter]

 >         at shell prompt will list file named 0xff (single char name
 >         with char value 255), not file named '?'.
 > +
 > +choice
 > +       prompt "LOOP_CONFIGURE or LOOP_SET_FD + LOOP_SET_STATUS"
 > +       default TRY_LOOP_CONFIGURE
 > +       help
 > +       LOOP_CONFIGURE is added to Linux 5.8
 > + https://lwn.net/Articles/820408/ 
 > +       This allows userspace to completely setup a loop device with 
a single
 > +       ioctl, removing the in-between state where the device can be 
partially
 > +       configured - eg the loop device has a backing file associated 
with it,

 > +       but is reading from the wrong offset.
 > +
 > +config LOOP_CONFIGURE
 > +       bool "always uses LOOP_CONFIGURE, kernel version >= 5.8"
 > +
 > +config NO_LOOP_CONFIGURE
 > +       bool "never uses LOOP_CONFIGURE, kernel version < 5.8"
 > +
 > +config TRY_LOOP_CONFIGURE
 > +       bool "try LOOP_CONFIGURE, kernel version is unknown"
 > +
 > +endchoice
 > diff --git a/libbb/loop.c b/libbb/loop.c
 > index ddb92fce3..be592bc4b 100644
 > --- a/libbb/loop.c
 > +++ b/libbb/loop.c
 > @@ -126,7 +126,8 @@ static int open_file(const char *file, unsigned 
flags, int *mode)

 >         return ffd;
 >  }
 >
 > -static int set_loop_configure(int ffd, int lfd, bb_loop_info *loopinfo)
 > +#ifndef CONFIG_LOOP_CONFIGURE
 > +static int set_loop_configure_old(int ffd, int lfd, bb_loop_info 
*loopinfo)

 >  {
 >         int rc;
 >         bb_loop_info loopinfo2;
 > @@ -149,6 +150,46 @@ static int set_loop_configure(int ffd, int lfd, 
bb_loop_info *loopinfo)

 >         ioctl(lfd, LOOP_CLR_FD, 0); // actually, 0 param is unnecessary
 >         return -1;
 >  }
 > +#endif
 > +
 > +#ifndef CONFIG_NO_LOOP_CONFIGURE
 > +
 > +#ifndef LOOP_CONFIGURE
 > +#define LOOP_CONFIGURE 0x4C0A
 > +struct loop_config {
 > +       uint32_t fd;
 > +       uint32_t block_size;
 > +       struct loop_info64 info;
 > +       uint64_t __reserved[8];
 > +};
 > +#endif
 > +
 > +/*
 > + * linux v5.8.0
 > + * loop: Add LOOP_CONFIGURE ioctl
 > + * https://lwn.net/Articles/820408/ 
 > + * 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3448914e8cc5 


 > + */
 > +static int set_loop_configure(int ffd, int lfd, bb_loop_info *loopinfo)
 > +{
 > +       int rc;
 > +       struct loop_config config;
 > +
 > +       memset(, 0, sizeof(config));
 > +       config.fd = ffd;
 > +       memcpy( , loopinfo, 

Re: [PATCH v2 6/9] loop:refactor: Use a structure to reduce parameters

[Xiaoming Ni]

On 2022/11/19 2:06, Kang-Che Sung wrote:



On Friday, November 18, 2022, Xiaoming Ni > wrote:

 > Step 6 of micro-refactoring the set_loop():
 >         Use structs to avoid transferring a large number of parameters
 >         in set_loop_configure() and set_loop_info()
 >
 > function                                             old     new   delta
 > set_loop                                             708     720     +12
 > 
--
 > (add/remove: 0/0 grow/shrink: 1/0 up/down: 12/0)               Total: 
12 bytes

 >
 > Signed-off-by: Xiaoming Ni >

 > ---
 >  libbb/loop.c | 54 +---
 >  1 file changed, 30 insertions(+), 24 deletions(-)
 >

...

 > -        */
 > -       loopinfo.lo_flags = (flags & ~BB_LO_FLAGS_READ_ONLY);
 > -       rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
 > -       if (rc != 0 && (loopinfo.lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
 > +       rc = ioctl(lfd, BB_LOOP_SET_STATUS, loopinfo);
 > +       if (rc != 0 && (loopinfo->lo_flags & BB_LO_FLAGS_AUTOCLEAR)) {
 > +               memcpy(, loopinfo, sizeof(*loopinfo));

Just use `loopinfo2 = loopinfo;`
Tested on my PC, there was no difference in code size between your 
modified scheme and my current scheme.


function old new   delta
--
(add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0) Total: 0 
bytes




Also, it is unclear why there is the need to clone the loopinfo buffer.

 >                 /* Old kernel, does not support LO_FLAGS_AUTOCLEAR? */
 >                 /* (this code path is not tested) */
 > -               loopinfo.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
 > -               rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
 > +               loopinfo2.lo_flags -= BB_LO_FLAGS_AUTOCLEAR;
 > +               rc = ioctl(lfd, BB_LOOP_SET_STATUS, );
 >         }
 >         if (rc == 0) {
 >                 return lfd;

...


 >
 > +static void init_bb_loop_info(bb_loop_info *loopinfo, const char *file,
 > +               unsigned long long offset, unsigned long long 
sizelimit, unsigned flags)

 > +{
 > +       memset(loopinfo, 0, sizeof(*loopinfo));

Would it reduce code size by doing the initialization like this?

```
        bb_loop_info empty = {0};
        loopinfo = empty;
```

Tested on my PC, there was no difference in code size between your 
modified scheme and my current scheme.


function old new   delta
--
(add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0) Total: 0 
bytes


Thanks
Xiaoming Ni
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

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

[Xiaoming Ni]

On 2022/11/19 1:40, Kang-Che Sung wrote:



On Friday, November 18, 2022, Xiaoming Ni > 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 >

 > ---
 >  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 */
 > +       }
 > +}

I'm a little nervous when the buffer length of `dev` is not passed into 
this function. Yes I know the buffer is large enough for the loop device 
path that would be printed. But I just wish there would be an assertion 
in this function, so that if the function is reused somewhere else, the 
developer would know what he is doing.



static int get_next_free_loop(char *dev, size_t dev_size, int id)
{
int loopdevno = get_free_loop();
if (loopdevno >= 0) {
snprintf(dev, dev_size, LOOP_FORMAT, loopdevno);
return 1; /* use /dev/loop-control */
}
if (loopdevno == -2) {
snprintf(dev, dev_size, LOOP_FORMAT, id);
return 2;
}
return -1; /* no free loop devices */
}

If the dev_size parameter is added to get_next_free_loop(), the code 
size increases, Is it worth?


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



Thanks
Xiaoming Ni
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox