2020-04-20 08:06:47

by Martijn Coenen

[permalink] [raw]
Subject: [PATCH 2/4] loop: Factor out configuring loop from status.

Factor out this code into a separate function, so it can be reused by
other code more easily.

Signed-off-by: Martijn Coenen <[email protected]>
---
drivers/block/loop.c | 108 +++++++++++++++++++++++--------------------
1 file changed, 58 insertions(+), 50 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d934d65dbe92..e0f9674fe568 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1276,12 +1276,68 @@ static int loop_clr_fd(struct loop_device *lo)
}

static int
-loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
{
int err;
struct loop_func_table *xfer;
kuid_t uid = current_uid();
+
+ if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
+ return -EINVAL;
+
+ err = loop_release_xfer(lo);
+ if (err)
+ return err;
+
+ if (info->lo_encrypt_type) {
+ unsigned int type = info->lo_encrypt_type;
+
+ if (type >= MAX_LO_CRYPT)
+ return -EINVAL;
+ xfer = xfer_funcs[type];
+ if (xfer == NULL)
+ return -EINVAL;
+ } else
+ xfer = NULL;
+
+ err = loop_init_xfer(lo, xfer, info);
+ if (err)
+ return err;
+
+ lo->lo_offset = info->lo_offset;
+ lo->lo_sizelimit = info->lo_sizelimit;
+ memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
+ memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
+ lo->lo_file_name[LO_NAME_SIZE-1] = 0;
+ lo->lo_crypt_name[LO_NAME_SIZE-1] = 0;
+
+ if (!xfer)
+ xfer = &none_funcs;
+ lo->transfer = xfer->transfer;
+ lo->ioctl = xfer->ioctl;
+
+ if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
+ (info->lo_flags & LO_FLAGS_AUTOCLEAR))
+ lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
+
+ lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
+ lo->lo_init[0] = info->lo_init[0];
+ lo->lo_init[1] = info->lo_init[1];
+ if (info->lo_encrypt_key_size) {
+ memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
+ info->lo_encrypt_key_size);
+ lo->lo_key_owner = uid;
+ }
+
+ return 0;
+}
+
+static int
+loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
+{
+ int err;
struct block_device *bdev;
+ kuid_t uid = current_uid();
bool partscan = false;
bool size_changed = false;
loff_t validated_size;
@@ -1299,10 +1355,6 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
err = -ENXIO;
goto out_unlock;
}
- if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE) {
- err = -EINVAL;
- goto out_unlock;
- }

if (lo->lo_offset != info->lo_offset ||
lo->lo_sizelimit != info->lo_sizelimit) {
@@ -1330,54 +1382,10 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
goto out_unfreeze;
}

- err = loop_release_xfer(lo);
- if (err)
- goto out_unfreeze;
-
- if (info->lo_encrypt_type) {
- unsigned int type = info->lo_encrypt_type;
-
- if (type >= MAX_LO_CRYPT) {
- err = -EINVAL;
- goto out_unfreeze;
- }
- xfer = xfer_funcs[type];
- if (xfer == NULL) {
- err = -EINVAL;
- goto out_unfreeze;
- }
- } else
- xfer = NULL;
-
- err = loop_init_xfer(lo, xfer, info);
+ err = loop_set_from_status(lo, info);
if (err)
goto out_unfreeze;

- lo->lo_offset = info->lo_offset;
- lo->lo_sizelimit = info->lo_sizelimit;
- memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
- memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
- lo->lo_file_name[LO_NAME_SIZE-1] = 0;
- lo->lo_crypt_name[LO_NAME_SIZE-1] = 0;
-
- if (!xfer)
- xfer = &none_funcs;
- lo->transfer = xfer->transfer;
- lo->ioctl = xfer->ioctl;
-
- if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
- (info->lo_flags & LO_FLAGS_AUTOCLEAR))
- lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
-
- lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
- lo->lo_init[0] = info->lo_init[0];
- lo->lo_init[1] = info->lo_init[1];
- if (info->lo_encrypt_key_size) {
- memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
- info->lo_encrypt_key_size);
- lo->lo_key_owner = uid;
- }
-
if (size_changed)
loop_set_size(lo, validated_size);

--
2.26.1.301.g55bc3eb7cb9-goog


2020-04-20 16:37:35

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH 2/4] loop: Factor out configuring loop from status.

On 4/20/20 1:04 AM, Martijn Coenen wrote:

No trailing dot at the end of a patch subject please.

> static int
> -loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
> +loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
> {
> int err;
> struct loop_func_table *xfer;
> kuid_t uid = current_uid();
> +
> + if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
> + return -EINVAL;
> +
> + err = loop_release_xfer(lo);
> + if (err)
> + return err;
> +
> + if (info->lo_encrypt_type) {
> + unsigned int type = info->lo_encrypt_type;
> +
> + if (type >= MAX_LO_CRYPT)
> + return -EINVAL;
> + xfer = xfer_funcs[type];
> + if (xfer == NULL)
> + return -EINVAL;
> + } else
> + xfer = NULL;
> +
> + err = loop_init_xfer(lo, xfer, info);
> + if (err)
> + return err;
> +
> + lo->lo_offset = info->lo_offset;
> + lo->lo_sizelimit = info->lo_sizelimit;
> + memcpy(lo->lo_file_name, info->lo_file_name, LO_NAME_SIZE);
> + memcpy(lo->lo_crypt_name, info->lo_crypt_name, LO_NAME_SIZE);
> + lo->lo_file_name[LO_NAME_SIZE-1] = 0;
> + lo->lo_crypt_name[LO_NAME_SIZE-1] = 0;
> +
> + if (!xfer)
> + xfer = &none_funcs;
> + lo->transfer = xfer->transfer;
> + lo->ioctl = xfer->ioctl;
> +
> + if ((lo->lo_flags & LO_FLAGS_AUTOCLEAR) !=
> + (info->lo_flags & LO_FLAGS_AUTOCLEAR))
> + lo->lo_flags ^= LO_FLAGS_AUTOCLEAR;
> +
> + lo->lo_encrypt_key_size = info->lo_encrypt_key_size;
> + lo->lo_init[0] = info->lo_init[0];
> + lo->lo_init[1] = info->lo_init[1];
> + if (info->lo_encrypt_key_size) {
> + memcpy(lo->lo_encrypt_key, info->lo_encrypt_key,
> + info->lo_encrypt_key_size);
> + lo->lo_key_owner = uid;
> + }
> +
> + return 0;
> +}

Please add a (one line?) comment above this function that explains the
purpose of this function. Is the purpose of this function perhaps to
initialize loop device parameters based on the information received from
user space (the 'info' argument)?

Thanks,

Bart.

2020-04-20 16:40:30

by Bart Van Assche

[permalink] [raw]
Subject: Re: [PATCH 2/4] loop: Factor out configuring loop from status.

On 4/20/20 1:04 AM, Martijn Coenen wrote:
> static int
> -loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
> +loop_set_from_status(struct loop_device *lo, const struct loop_info64 *info)
> {

An additional question: since this function sets the status of 'lo'
based on the information in 'info', would "loop_set_status" or
"loop_set_status_from_info" be a better name for this function?

Thanks,

Bart.

2020-04-21 11:48:28

by Martijn Coenen

[permalink] [raw]
Subject: Re: [PATCH 2/4] loop: Factor out configuring loop from status.

On Mon, Apr 20, 2020 at 3:49 PM Bart Van Assche <[email protected]> wrote:
>
> An additional question: since this function sets the status of 'lo'
> based on the information in 'info', would "loop_set_status" or
> "loop_set_status_from_info" be a better name for this function?

Yeah, I like the latter. I will rename, and add a comment that
explains the purpose more clearly.

Thanks,
Martijn

>
> Thanks,
>
> Bart.