On Friday, November 18, 2022, Xiaoming Ni <[email protected]> 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 <[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 */
> + }
> +}
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 */
> /* 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
