linux-kernel - Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable "nooutpages" in cryptocop_ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 28 Aug 2016 12:24:40 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     SF Markus Elfring <elfring@...rs.sourceforge.net>
cc:     Julia Lawall <julia.lawall@...6.fr>, linux-cris-kernel@...s.com,
        Adam Buchbinder <adam.buchbinder@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...nel.org>,
        Jesper Nilsson <jesper.nilsson@...s.com>,
        Jiri Kosina <jkosina@...e.cz>,
        Mikael Starvik <starvik@...s.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH 5/8] cris-cryptocop: Move an assignment for the variable
 "nooutpages" in cryptocop_ioctl_process()



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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>