[PATCH] efivarfs: 'efivarfs_file_write' function reorganization

[Geyslan G. Bem]

This reorganization:

Adds 'attrsize' variable to make the code cleaner and more
understandable, replacing all 'sizeof(attributes)'.

Removes 'bytes' prior assignment due this new approach.

Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---
fs/efivarfs/file.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 8dd524f..e190aa0 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -18,29 +18,24 @@ static ssize_t efivarfs_file_write(struct file *file,
{
struct efivar_entry *var = file->private_data;
void *data;
- u32 attributes;
+ u32 attributes, attrsize = sizeof(attributes);
struct inode *inode = file->f_mapping->host;
- unsigned long datasize = count - sizeof(attributes);
- ssize_t bytes = 0;
+ unsigned long datasize = count - attrsize;
+ ssize_t bytes;
bool set = false;

- if (count < sizeof(attributes))
+ if (count < attrsize)
return -EINVAL;

- if (copy_from_user(&attributes, userbuf, sizeof(attributes)))
+ if (copy_from_user(&attributes, userbuf, attrsize))
return -EFAULT;

if (attributes & ~(EFI_VARIABLE_MASK))
return -EINVAL;

- data = kmalloc(datasize, GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
- bytes = -EFAULT;
- goto out;
- }
+ data = memdup_user(userbuf + attrsize, datasize);
+ if (IS_ERR(data))
+ return PTR_ERR(data);

bytes = efivar_entry_set_get_size(var, attributes, &datasize,
data, &set);
@@ -56,7 +51,7 @@ static ssize_t efivarfs_file_write(struct file *file,
dput(file->f_dentry);
} else {
mutex_lock(&inode->i_mutex);
- i_size_write(inode, datasize + sizeof(attributes));
+ i_size_write(inode, datasize + attrsize);
mutex_unlock(&inode->i_mutex);
}

--
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Matt Fleming]

On Mon, 14 Oct, at 03:37:17PM, Geyslan G. Bem wrote:
> This reorganization:
>
> Adds 'attrsize' variable to make the code cleaner and more
> understandable, replacing all 'sizeof(attributes)'.
>
> Removes 'bytes' prior assignment due this new approach.
>
> Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'.
>
> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
> ---
> fs/efivarfs/file.c | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)

Hmm.. I'm not convinced this is much of an improvement. I think removing
'sizeof(attributes)' actually makes the code harder to read.

--
Matt Fleming, Intel Open Source Technology Center

[Geyslan Gregório Bem]

2013/10/30 Matt Fleming <ma...@console-pimps.org>:

> On Mon, 14 Oct, at 03:37:17PM, Geyslan G. Bem wrote:
>> This reorganization:
>>
>> Adds 'attrsize' variable to make the code cleaner and more
>> understandable, replacing all 'sizeof(attributes)'.
>>
>> Removes 'bytes' prior assignment due this new approach.
>>
>> Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'.
>>
>> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
>> ---
>> fs/efivarfs/file.c | 23 +++++++++--------------
>> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> Hmm.. I'm not convinced this is much of an improvement. I think removing
> 'sizeof(attributes)' actually makes the code harder to read.
>
> --
> Matt Fleming, Intel Open Source Technology Center

Do you want that I undo that? I aggre that the variable use only
reduces the line code.

--
Regards,

Geyslan G. Bem
hackingbits.com

[Matt Fleming]

On Wed, 30 Oct, at 10:44:16AM, Geyslan Gregório Bem wrote:
> Do you want that I undo that? I aggre that the variable use only
> reduces the line code.

Yes please.

--
Matt Fleming, Intel Open Source Technology Center

[Geyslan G. Bem]

This reorganization:

Removes useless 'bytes' prior assignment.

Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
---

fs/efivarfs/file.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c
index 8dd524f..cdb2971 100644
--- a/fs/efivarfs/file.c
+++ b/fs/efivarfs/file.c
@@ -21,7 +21,7 @@ static ssize_t efivarfs_file_write(struct file *file,
u32 attributes;

struct inode *inode = file->f_mapping->host;

unsigned long datasize = count - sizeof(attributes);
- ssize_t bytes = 0;

+ ssize_t bytes;
bool set = false;

if (count < sizeof(attributes))
@@ -33,14 +33,9 @@ static ssize_t efivarfs_file_write(struct file *file,

if (attributes & ~(EFI_VARIABLE_MASK))
return -EINVAL;

- data = kmalloc(datasize, GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
- if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
- bytes = -EFAULT;
- goto out;
- }

+ data = memdup_user(userbuf + sizeof(attributes), datasize);

+ if (IS_ERR(data))
+ return PTR_ERR(data);

bytes = efivar_entry_set_get_size(var, attributes, &datasize,
data, &set);

--
1.8.4

[Geyslan Gregório Bem]

2013/10/30 Matt Fleming <ma...@console-pimps.org>:

> On Wed, 30 Oct, at 10:44:16AM, Geyslan Gregório Bem wrote:
>> Do you want that I undo that? I aggre that the variable use only
>> reduces the line code.
>
> Yes please.
>
> --
> Matt Fleming, Intel Open Source Technology Center

Done:
[PATCH v2] efivarfs: 'efivarfs_file_write' function reorganization

--
Regards,

Geyslan G. Bem
hackingbits.com

[Matt Fleming]

On Wed, 30 Oct, at 03:57:41PM, Geyslan G. Bem wrote:
> This reorganization:
>
> Removes useless 'bytes' prior assignment.
>
> Uses 'memdup_user' instead 'kmalloc' + 'copy_from_user'.
>
> Signed-off-by: Geyslan G. Bem <gey...@gmail.com>
> ---
> fs/efivarfs/file.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)

Thanks, this is what I applied,

---

From 00ad8da13638dba692eaa58b0cba25f3f1a801b1 Mon Sep 17 00:00:00 2001
From: "Geyslan G. Bem" <gey...@gmail.com>
Date: Wed, 30 Oct 2013 15:57:41 -0300
Subject: [PATCH] efivarfs: 'efivarfs_file_write' function reorganization

This reorganization removes useless 'bytes' prior assignment and uses

'memdup_user' instead 'kmalloc' + 'copy_from_user'.

Signed-off-by: Geyslan G. Bem <gey...@gmail.com>

Signed-off-by: Matt Fleming <matt.f...@intel.com>

---
fs/efivarfs/file.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/efivarfs/file.c b/fs/efivarfs/file.c

index 8dd524f32284..cdb2971192a5 100644

1.8.1.4

--
Matt Fleming, Intel Open Source Technology Center