Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1470688 > unrolled thread
| Started by | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| First post | 2016-08-26 15:00 +0200 |
| Last post | 2016-08-28 12:30 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-26 15:00 +0200
Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() Julia Lawall <julia.lawall@lip6.fr> - 2016-08-27 21:10 +0200
Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-08-28 09:30 +0200
Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() Julia Lawall <julia.lawall@lip6.fr> - 2016-08-28 12:30 +0200
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-08-26 15:00 +0200 |
| Subject | [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl_process() |
| Message-ID | <saoTL-7aM-9@gated-at.bofh.it> |
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 26 Aug 2016 13:38:30 +0200
Move the assignment for the local variable "nooutpages" behind
the source code for memory allocations by this function.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
arch/cris/arch-v32/drivers/cryptocop.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/cris/arch-v32/drivers/cryptocop.c b/arch/cris/arch-v32/drivers/cryptocop.c
index 26347a2..cd34723 100644
--- a/arch/cris/arch-v32/drivers/cryptocop.c
+++ b/arch/cris/arch-v32/drivers/cryptocop.c
@@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
struct page **inpages = NULL;
struct page **outpages = NULL;
int noinpages = 0;
- int nooutpages = 0;
+ int nooutpages;
struct cryptocop_desc descs[5]; /* Max 5 descriptors are needed, there are three transforms that
* can get connected/disconnected on different places in the indata. */
@@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
err = -ENOMEM;
goto free_inpages;
}
+ } else {
+ nooutpages = 0;
}
/* Acquire the mm page semaphore. */
--
2.9.3
[toc] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2016-08-27 21:10 +0200 |
| Message-ID | <saR9n-8pi-1@gated-at.bofh.it> |
| In reply to | #1470688 |
On Fri, 26 Aug 2016, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 26 Aug 2016 13:38:30 +0200
>
> Move the assignment for the local variable "nooutpages" behind
> the source code for memory allocations by this function.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> arch/cris/arch-v32/drivers/cryptocop.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/cris/arch-v32/drivers/cryptocop.c b/arch/cris/arch-v32/drivers/cryptocop.c
> index 26347a2..cd34723 100644
> --- a/arch/cris/arch-v32/drivers/cryptocop.c
> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
> struct page **inpages = NULL;
> struct page **outpages = NULL;
> int noinpages = 0;
> - int nooutpages = 0;
> + int nooutpages;
>
> struct cryptocop_desc descs[5]; /* Max 5 descriptors are needed, there are three transforms that
> * can get connected/disconnected on different places in the indata. */
> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
> err = -ENOMEM;
> goto free_inpages;
> }
> + } else {
> + nooutpages = 0;
Why is it better? 4 characters have becomes 2 lines.
julia
> }
>
> /* Acquire the mm page semaphore. */
> --
> 2.9.3
>
>
[toc] | [prev] | [next] | [standalone]
| From | SF Markus Elfring <elfring@users.sourceforge.net> |
|---|---|
| Date | 2016-08-28 09:30 +0200 |
| Message-ID | <sb2Hv-77Y-1@gated-at.bofh.it> |
| In reply to | #1471240 |
>> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
>> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
>> struct page **inpages = NULL;
>> struct page **outpages = NULL;
>> int noinpages = 0;
>> - int nooutpages = 0;
>> + int nooutpages;
>>
>> struct cryptocop_desc descs[5]; /* Max 5 descriptors are needed, there are three transforms that
>> * can get connected/disconnected on different places in the indata. */
>> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
>> err = -ENOMEM;
>> goto free_inpages;
>> }
>> + } else {
>> + nooutpages = 0;
>
> Why is it better? 4 characters have becomes 2 lines.
I suggest to express in a more precise way where this variable is needed actually.
* It would also be an update candidate for the refactoring "Reduce the scope of a variable", wouldn't it?
* Or would the refactoring "Split the implementation of a function into further functions" more appropriate here?
Regards,
Markus
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2016-08-28 12:30 +0200 |
| Message-ID | <sb5vI-pw-17@gated-at.bofh.it> |
| In reply to | #1471298 |
On Sun, 28 Aug 2016, SF Markus Elfring wrote:
> >> +++ b/arch/cris/arch-v32/drivers/cryptocop.c
> >> @@ -2469,7 +2469,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
> >> struct page **inpages = NULL;
> >> struct page **outpages = NULL;
> >> int noinpages = 0;
> >> - int nooutpages = 0;
> >> + int nooutpages;
> >>
> >> struct cryptocop_desc descs[5]; /* Max 5 descriptors are needed, there are three transforms that
> >> * can get connected/disconnected on different places in the indata. */
> >> @@ -2695,6 +2695,8 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig
> >> err = -ENOMEM;
> >> goto free_inpages;
> >> }
> >> + } else {
> >> + nooutpages = 0;
> >
> > Why is it better? 4 characters have becomes 2 lines.
>
> I suggest to express in a more precise way where this variable is needed
> actually.
The variable is used in the cleanup code at the end of the function.
Thus it conceptually has global scope, and it is completely reasonable to
initialize it at the beginning of its function, along with noinpages.
This code is horrible in so many ways: no space before {, lots of 0
initializations instead of kzalloc, random use of local cleanup code and
a label at the end of the function, the use of DEBUG, the use of printk,
the use of the very long function name in strings instead of __func__,
constants on the left of a != test, etc. On the other hand there are also
very few commits on this code, and even fewer that are specific to this
code, so perhaps no one cares about it.
julia
>
> * It would also be an update candidate for the refactoring "Reduce the scope of a variable", wouldn't it?
>
> * Or would the refactoring "Split the implementation of a function into further functions" more appropriate here?
>
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web