[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH v4 1/9] loop:refactor: extract subfunction open_file()
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2022-12-12 15:22:19
Message-ID: CAK1hOcPX6mjfxsmYE_VxfhAZCNhHUQ_6a4VRcK8rFVZETw=LCA () mail ! gmail ! com
[Download RAW message or body]

On Mon, Nov 21, 2022 at 2:58 PM Xiaoming Ni <nixiaoming@huawei.com> wrote:
> +static int open_file(const char *file, unsigned flags, int *mode)
> +{
> +       int ffd;
> +       /* open the file.  barf if this doesn't work.  */
> +       *mode = (flags & BB_LO_FLAGS_READ_ONLY) ? O_RDONLY : O_RDWR;
> + retry_open_ffd:
> +       ffd = open(file, *mode);
> +       if (ffd < 0) {
> +               if (*mode != O_RDONLY) {
> +                       *mode = O_RDONLY;
> +                       goto retry_open_ffd;
> +               }
> +       }
> +       return ffd;
> +}
> +
>  /* Returns opened fd to the loop device, <0 on error.
>   * *device is loop device to use, or if *device==NULL finds a loop device to
>   * mount it on and sets *device to a strdup of that loop device name.
> @@ -109,15 +125,8 @@ int FAST_FUNC set_loop(char **device, const char *file, unsigned long long offse
>         struct stat statbuf;
>         int i, lfd, ffd, mode, rc;
>
> -       /* Open the file.  Barf if this doesn't work.  */
> -       mode = (flags & BB_LO_FLAGS_READ_ONLY) ? O_RDONLY : O_RDWR;
> - open_ffd:
> -       ffd = open(file, mode);
> +       ffd = open_file(file, flags, &mode);
>         if (ffd < 0) {
> -               if (mode != O_RDONLY) {
> -                       mode = O_RDONLY;
> -                       goto open_ffd;
> -               }

open_file() name is not describing what function actually does -
"open RW or RO".

The change seems to not be needed by any further patches.

The change makes the code less efficient by now requiring "mode"
to live on stack, unless compiler inlines the single-use static
and figures out that in fact, the "&mode" can be proven to not need
a memory-addressable location. The current compilers
usually are good enough to do so, but why do it anyway?
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]