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

[Xiaoming Ni]

On 2022/11/21 15:50, Kang-Che Sung wrote:

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);`
.



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 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 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

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

[Kang-Che Sung]

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 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 

[PATCH v2 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..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 */
/* 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 (rc == 0