AW: [PATCH 2/9] loop:refactor: extract subfunction get_next_free_loop()
Xiaoming Ni
nixiaoming at huawei.com
Fri Nov 18 12:36:34 UTC 2022
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 <busybox-bounces at busybox.net> im Auftrag von Xiaoming Ni <nixiaoming at huawei.com>
> Gesendet: Freitag, 18. November 2022 02:01:51
> An: busybox at busybox.net; vda.linux at googlemail.com
> Cc: wangle6 at huawei.com
> 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 <nixiaoming at huawei.com>
> ---
> 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
> busybox at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
> .
>
More information about the busybox
mailing list