| Message ID | 247c0e27-c442-3408-4f92-492629d61fbf@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/15/2017 7:04 AM, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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 <elfring@users.sourceforge.net> 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; > } > > /* -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 15, 2017 at 10:04 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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 <elfring@users.sourceforge.net> > --- > security/selinux/ss/policydb.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) Like you, I generally despise the notion of unconditionally setting return codes to an error value *before* the faulting call; see Eric's response on patch 0/46, my preferred style is different from Eric. However, I agree with Casey that this patch is mostly just code churn so I'm going to drop this from your series. > 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; > } > > /* > -- > 2.11.0 >
> However, I agree with Casey that this patch is mostly just code churn > so I'm going to drop this from your series. How do you think about to return only constant error codes in this function? Would it be acceptable to replace any statements “goto out;” with “return -ENOMEM;” here instead? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 27, 2017 at 2:24 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >> However, I agree with Casey that this patch is mostly just code churn >> so I'm going to drop this from your series. > > How do you think about to return only constant error codes in this function? > Would it be acceptable to replace any statements “goto out;” with > “return -ENOMEM;” here instead? Yes.
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; } /*