tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl
PATCH v2 1/1 tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl
Hoist 'len' and use it in both cases.
Add a comment explaining why reassigning 'kbs' is intentional.
---
Changes in v2:
- Keep 'kbs' reassignment and add a comment why it's required (Jiri)
- Link to v1: https://lore.kernel.org/lkml/[email protected]/
--- drivers/tty/vt/keyboard.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/tty/vt/keyboard.c +10 -4
@@ -2000,17 +2000,18 @@ static char *vt_kdskbsent(char *kbs, unsigned char cur)int vt_do_kdgkb_ioctl (int cmd,struct kbsentry __user *user_kdgkb,int perm) {unsigned char kb_func; +ssize_t len;if (get_user (kb_func, &user_kdgkb->kb_func))return -EFAULT ; kb_func =array_index_nospec (kb_func,MAX_NR_FUNC ); +/* size should have been a struct member */ + len =sizeof (user_kdgkb->kb_string); +switch (cmd) {case KDGKBSENT : { -/* size should have been a struct member */ -ssize_t len =sizeof (user_kdgkb->kb_string); - char __free(kfree) *kbs = kmalloc(len, GFP_KERNEL);if (!kbs)return -ENOMEM ;
@@ -2031,11 +2032,16 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)return -EPERM ; char __free(kfree) *kbs = strndup_user(user_kdgkb->kb_string, -sizeof (user_kdgkb->kb_string)); + len);if (IS_ERR (kbs))return PTR_ERR (kbs);guard (spinlock_irqsave)(&func_buf_lock); + +/* +* Ownership transfer: vt_kdskbsent() returns a pointer +* that must be freed (new buffer, old buffer, or NULL). +*/ kbs =vt_kdskbsent (kbs, kb_func);return 0 ;
Signature · 2 lines
Thorsten Blum <[email protected]> GPG: 1D60 735E 8AEF 3BE4 73B6 9D84 7336 78FD 8DFE EAD4
Greg Kroah-Hartman
maintainerQuoted 1 line · Thorsten Blum
Hoist 'len' and use it in both cases.
Why? And what is "both cases"?
Quoted 10 lines
Add a comment explaining why reassigning 'kbs' is intentional.
Signed-off-by Thorsten Blum <[email protected]>---
Changes in v2:
- Keep 'kbs' reassignment and add a comment why it's required (Jiri)
- Link to v1: https://lore.kernel.org/lkml/[email protected]/--- drivers/tty/vt/keyboard.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
I feel you just made the code harder to understand, as you added complexity :(
Quoted 33 lines
drivers/tty/vt/keyboard.c +5
-4@@ -2000,17 +2000,18 @@ static char *vt_kdskbsent(char *kbs, unsigned char cur)int vt_do_kdgkb_ioctl (int cmd,struct kbsentry __user *user_kdgkb,int perm) {unsigned char kb_func; +ssize_t len;if (get_user (kb_func, &user_kdgkb->kb_func))return -EFAULT ; kb_func =array_index_nospec (kb_func,MAX_NR_FUNC ); +/* size should have been a struct member */ + len =sizeof (user_kdgkb->kb_string); +switch (cmd) {case KDGKBSENT : { -/* size should have been a struct member */ -ssize_t len =sizeof (user_kdgkb->kb_string); - char __free(kfree) *kbs = kmalloc(len, GFP_KERNEL);if (!kbs)return -ENOMEM ;@@ -2031,11 +2032,16 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)return -EPERM ; char __free(kfree) *kbs = strndup_user(user_kdgkb->kb_string, -sizeof (user_kdgkb->kb_string)); + len);
drivers/tty/vt/keyboard.c int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) · line 2045
Quoted 10 lines
drivers/tty/vt/keyboard.c +5
-0if (IS_ERR (kbs))return PTR_ERR (kbs);guard (spinlock_irqsave)(&func_buf_lock); + +/* +* Ownership transfer: vt_kdskbsent() returns a pointer +* that must be freed (new buffer, old buffer, or NULL). +*/ kbs =vt_kdskbsent (kbs, kb_func);
That's fine, but what does it have to do with len?
confused,
greg k-h
Quoted 4 lines · Greg Kroah-Hartman
Quoted 1 line · Thorsten Blum
Hoist 'len' and use it in both cases.
Why? And what is "both cases"?
To reuse 'len' in both switch cases (KDGKBSENT and KDSKBSENT) instead of defining 'len = sizeof(user_kdgkb->kb_string)' in KDGKBSENT and inlining sizeof(user_kdgkb->kb_string) in KDSKBSENT.
Quoted 11 lines
Quoted 10 lines
Add a comment explaining why reassigning 'kbs' is intentional.
Signed-off-by Thorsten Blum <[email protected]>---
Changes in v2:
- Keep 'kbs' reassignment and add a comment why it's required (Jiri)
- Link to v1: https://lore.kernel.org/lkml/[email protected]/
---drivers/tty/vt/keyboard.c| 14 ++++++++++----1 file changed, 10 insertions(+), 4 deletions(-)
Quoted 2 lines
I feel you just made the code harder to understand, as you added complexity :(
Not sure how reusing a local variable adds complexity? Would renaming 'len' to 'kb_string_len' help?
Quoted 9 lines
Quoted 7 lines
guard(spinlock_irqsave)(&func_buf_lock);drivers/tty/vt/keyboard.c +5
-0+ +/* +* Ownership transfer: vt_kdskbsent() returns a pointer +* that must be freed (new buffer, old buffer, or NULL). +*/ kbs = vt_kdskbsent(kbs, kb_func);That's fine, but what does it have to do with len?
It's unrelated to 'len' and just a drive-by change while I was at it.
Thanks,
Thorsten
Greg Kroah-Hartman
maintainerQuoted 9 lines · Thorsten Blum
Quoted 4 lines · Greg Kroah-Hartman
Quoted 1 line · Thorsten Blum
Hoist 'len' and use it in both cases.
Why? And what is "both cases"?
To reuse 'len' in both switch cases (KDGKBSENT and KDSKBSENT) instead of defining 'len =
sizeof(user_kdgkb->kb_string)' in KDGKBSENT and inliningsizeof(user_kdgkb->kb_string)in KDSKBSENT.
As the sizeof() turns into a static number, the code is a bit simpler as-is, right?
And there's no real need to change this for the sake of changing it that I can see.
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
drivers/tty/vt/keyboard.c +4
-0+/* +* Ownership transfer: vt_kdskbsent() returns a pointer +* that must be freed (new buffer, old buffer, or NULL). +*/ kbs = vt_kdskbsent(kbs, kb_func);That's fine, but what does it have to do with len?
It's unrelated to 'len' and just a drive-by change while I was at it.
Which, by default, makes this patch impossible to accept :(
thanks,
greg k-h