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, &loopinfo);
>    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, &loopinfo);
> }
>   if (rc == 0)
>               return lfd;
> 
> +       rc = ioctl(lfd, BB_LOOP_SET_STATUS, &loopinfo);
> +       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, &loopinfo);
> +       }
> +       if (rc == 0) {
> +               return lfd;
> +       }

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

Thanks
Xiaoming Ni


> ________________________________________
> Von: busybox <busybox-bounces at busybox.net> im Auftrag von Xiaoming Ni <nixiaoming at huawei.com>
> Gesendet: Freitag, 18. November 2022 13:14:44
> An: busybox at busybox.net; vda.linux at googlemail.com; cand at gmx.com; explorer09 at gmail.com
> Cc: wangle6 at 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 <nixiaoming at huawei.com>
> ---
>   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(&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);
> +       rc = ioctl(lfd, BB_LOOP_SET_STATUS, &loopinfo);
> +       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, &loopinfo);
> +       }
> +       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(&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);
> -               rc = ioctl(lfd, BB_LOOP_SET_STATUS, &loopinfo);
> -               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, &loopinfo);
> -               }
> -               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 at busybox.net
> http://lists.busybox.net/mailman/listinfo/busybox
> .
>