From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751331AbdAQQbJ (ORCPT ); Tue, 17 Jan 2017 11:31:09 -0500 Received: from nm8-vm0.bullet.mail.ne1.yahoo.com ([98.138.91.23]:46088 "EHLO nm8-vm0.bullet.mail.ne1.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751052AbdAQQbH (ORCPT ); Tue, 17 Jan 2017 11:31:07 -0500 X-Yahoo-Newman-Id: 967622.17585.bm@smtp109.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 83THXyUVM1n4H3W67bvIh3ZlnW0SKSwr1dvQzf83..wDe7d 5dMPGuPYbXPcrbBDOloD7672qQjJTDIicVTJgUhxHOYzTK24slN71K4TjJT8 c8wVXplZ29s5u5hDwZx8X9jldhCuVLOxwe5ODDRmK68VO7iJZKKPojd1wu2T WLupW_tIW37uT8mIgRtqArQQlJw6MMKwS.9BWP8z_O1.Ku.x2fq6Pq2aC59a px8PtvOyy_sBFtu6Uy7kSLpLE17r3cNJVt3rNyl3fAsx9yhRi2pePCPg6.i4 QRrCd2SV9M1Y5SQfn3HVRpt9TMpDqyOFiSa2tOCiLA8QOFo3kGEEFuKItENA DqsZoJwaFGBg8Zqn0RRQDPFHK7DCnTqom.6Fi_qVqHynpaCZzdHowOZIQ9W4 DOAVEYljr.LEJ1CiFpVo6D3lYjcUIgtAXPom7msyiYhAH4drmPeconiYYW96 TaxkTTWtU94g5BYwQ.D.vU1anu7kvTiAfet7NKAZUuoqwK.iP7lqsKH.z7kT Jy_HZv23L1smvyPsbK7Z4hRMcFHGcvvpekiJ0ZC3QRF7wLkGEercaU_k- X-Yahoo-SMTP: OIJXglSswBDfgLtXluJ6wiAYv6_cnw-- Subject: Re: [PATCH 07/46] selinux: Delete unnecessary variable assignments in policydb_index() To: SF Markus Elfring , linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov, Eric Paris , James Morris , Paul Moore , "Serge E. Hallyn" , Stephen Smalley , William Roberts References: <247c0e27-c442-3408-4f92-492629d61fbf@users.sourceforge.net> Cc: LKML , kernel-janitors@vger.kernel.org From: Casey Schaufler Message-ID: <16b4fb1b-a6b4-a84c-3541-ca6a156d651e@schaufler-ca.com> Date: Tue, 17 Jan 2017 08:27:38 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <247c0e27-c442-3408-4f92-492629d61fbf@users.sourceforge.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/15/2017 7:04 AM, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sat, 14 Jan 2017 13:40:25 +0100 > > The local variable "rc" was reset with an error code up to five times > before a memory allocation failure was detected. > > Add a jump target so that this assignment will only be performed after > a concrete software failure. > > Signed-off-by: Markus Elfring Of course the maintainers get the call on this, but I see this as an example of the kind of code that led to the "gotos considered harmful" mindset. What value does it add? All I see is unnecessary code churn. And a backwards goto. > --- > security/selinux/ss/policydb.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 21869b622c0c..4d4ba1ad910d 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -539,34 +539,30 @@ static int policydb_index(struct policydb *p) > symtab_hash_eval(p->symtab); > #endif > > - rc = -ENOMEM; > p->class_val_to_struct = kcalloc(p->p_classes.nprim, > sizeof(*p->class_val_to_struct), > GFP_KERNEL); > if (!p->class_val_to_struct) > - goto out; > + goto failure_indication; > > - rc = -ENOMEM; > p->role_val_to_struct = kcalloc(p->p_roles.nprim, > sizeof(*p->role_val_to_struct), > GFP_KERNEL); > if (!p->role_val_to_struct) > - goto out; > + goto failure_indication; > > - rc = -ENOMEM; > p->user_val_to_struct = kcalloc(p->p_users.nprim, > sizeof(*p->user_val_to_struct), > GFP_KERNEL); > if (!p->user_val_to_struct) > - goto out; > + goto failure_indication; > > /* Yes, I want the sizeof the pointer, not the structure */ > - rc = -ENOMEM; > p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *), > p->p_types.nprim, > GFP_KERNEL | __GFP_ZERO); > if (!p->type_val_to_struct_array) > - goto out; > + goto failure_indication; > > rc = flex_array_prealloc(p->type_val_to_struct_array, 0, > p->p_types.nprim, GFP_KERNEL | __GFP_ZERO); > @@ -578,12 +574,11 @@ static int policydb_index(struct policydb *p) > goto out; > > for (i = 0; i < SYM_NUM; i++) { > - rc = -ENOMEM; > p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *), > p->symtab[i].nprim, > GFP_KERNEL | __GFP_ZERO); > if (!p->sym_val_to_name[i]) > - goto out; > + goto failure_indication; > > rc = flex_array_prealloc(p->sym_val_to_name[i], > 0, p->symtab[i].nprim, > @@ -598,6 +593,9 @@ static int policydb_index(struct policydb *p) > rc = 0; > out: > return rc; > +failure_indication: > + rc = -ENOMEM; > + goto out; > } > > /* From mboxrd@z Thu Jan 1 00:00:00 1970 From: Casey Schaufler Date: Tue, 17 Jan 2017 16:27:38 +0000 Subject: Re: [PATCH 07/46] selinux: Delete unnecessary variable assignments in policydb_index() Message-Id: <16b4fb1b-a6b4-a84c-3541-ca6a156d651e@schaufler-ca.com> List-Id: References: <247c0e27-c442-3408-4f92-492629d61fbf@users.sourceforge.net> In-Reply-To: <247c0e27-c442-3408-4f92-492629d61fbf@users.sourceforge.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: SF Markus Elfring , linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov, Eric Paris , James Morris , Paul Moore , "Serge E. Hallyn" , Stephen Smalley , William Roberts Cc: LKML , kernel-janitors@vger.kernel.org On 1/15/2017 7:04 AM, SF Markus Elfring wrote: > From: Markus Elfring > Date: Sat, 14 Jan 2017 13:40:25 +0100 > > The local variable "rc" was reset with an error code up to five times > before a memory allocation failure was detected. > > Add a jump target so that this assignment will only be performed after > a concrete software failure. > > Signed-off-by: Markus Elfring Of course the maintainers get the call on this, but I see this as an example of the kind of code that led to the "gotos considered harmful" mindset. What value does it add? All I see is unnecessary code churn. And a backwards goto. > --- > security/selinux/ss/policydb.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 21869b622c0c..4d4ba1ad910d 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -539,34 +539,30 @@ static int policydb_index(struct policydb *p) > symtab_hash_eval(p->symtab); > #endif > > - rc = -ENOMEM; > p->class_val_to_struct = kcalloc(p->p_classes.nprim, > sizeof(*p->class_val_to_struct), > GFP_KERNEL); > if (!p->class_val_to_struct) > - goto out; > + goto failure_indication; > > - rc = -ENOMEM; > p->role_val_to_struct = kcalloc(p->p_roles.nprim, > sizeof(*p->role_val_to_struct), > GFP_KERNEL); > if (!p->role_val_to_struct) > - goto out; > + goto failure_indication; > > - rc = -ENOMEM; > p->user_val_to_struct = kcalloc(p->p_users.nprim, > sizeof(*p->user_val_to_struct), > GFP_KERNEL); > if (!p->user_val_to_struct) > - goto out; > + goto failure_indication; > > /* Yes, I want the sizeof the pointer, not the structure */ > - rc = -ENOMEM; > p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *), > p->p_types.nprim, > GFP_KERNEL | __GFP_ZERO); > if (!p->type_val_to_struct_array) > - goto out; > + goto failure_indication; > > rc = flex_array_prealloc(p->type_val_to_struct_array, 0, > p->p_types.nprim, GFP_KERNEL | __GFP_ZERO); > @@ -578,12 +574,11 @@ static int policydb_index(struct policydb *p) > goto out; > > for (i = 0; i < SYM_NUM; i++) { > - rc = -ENOMEM; > p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *), > p->symtab[i].nprim, > GFP_KERNEL | __GFP_ZERO); > if (!p->sym_val_to_name[i]) > - goto out; > + goto failure_indication; > > rc = flex_array_prealloc(p->sym_val_to_name[i], > 0, p->symtab[i].nprim, > @@ -598,6 +593,9 @@ static int policydb_index(struct policydb *p) > rc = 0; > out: > return rc; > +failure_indication: > + rc = -ENOMEM; > + goto out; > } > > /*