On 2022/11/18 20:20, Walter Harms wrote:
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 ...
Thanks, I'll send a v3 patch later to replace "i" with "loopdevno",
refer to the variable name of get_free_loop().
Also, I made a mistake in patch9's commit msg.
The log of the code size change is misposted.
I will send patch v3 later
Thanks
________________________________________
Von: busybox <[email protected]> im Auftrag von Xiaoming Ni
<[email protected]>
Gesendet: Freitag, 18. November 2022 02:01:51
An: [email protected]; [email protected]
Cc: [email protected]
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 <[email protected]>
---
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
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
.
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
