| Message ID | 7fdcfc8f-affa-fcf8-adaf-dc8fd9e1b472@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Sun, Jan 15, 2017 at 10:10 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 14 Jan 2017 15:22:29 +0100 > > One local variable was set to an error code in some cases before > a concrete error situation was detected. Thus move the corresponding > assignments into if branches to indicate a software failure there. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 24 deletions(-) More code churn with no real advantage. I agree with the style you are using, and would support changing it if you are in the function fixing bugs or doing other substantial changes in that code, but I can't justify it as a standalone change, sorry. > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 53e6d06e772a..506b0228d1f1 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -2250,15 +2250,14 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -EINVAL; > if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) { > printk(KERN_ERR "SELinux: policydb magic number 0x%x does " > "not match expected magic number 0x%x\n", > le32_to_cpu(buf[0]), POLICYDB_MAGIC); > + rc = -EINVAL; > goto bad; > } > > - rc = -EINVAL; > len = le32_to_cpu(buf[1]); > if (len != strlen(POLICYDB_STRING)) { > printk(KERN_ERR "SELinux: policydb string length %d does not " > @@ -2265,11 +2265,13 @@ int policydb_read(struct policydb *p, void *fp) > len, strlen(POLICYDB_STRING)); > + rc = -EINVAL; > goto bad; > } > > - rc = -ENOMEM; > policydb_str = kmalloc(len + 1, GFP_KERNEL); > - if (!policydb_str) > + if (!policydb_str) { > + rc = -ENOMEM; > goto bad; > + } > > rc = next_entry(policydb_str, fp, len); > if (rc) { > @@ -2279,12 +2280,12 @@ int policydb_read(struct policydb *p, void *fp) > goto bad; > } > > - rc = -EINVAL; > policydb_str[len] = '\0'; > if (strcmp(policydb_str, POLICYDB_STRING)) { > printk(KERN_ERR "SELinux: policydb string %s does not match " > "my string %s\n", policydb_str, POLICYDB_STRING); > kfree(policydb_str); > + rc = -EINVAL; > goto bad; > } > /* Done with policydb_str. */ > @@ -2296,24 +2297,24 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -EINVAL; > p->policyvers = le32_to_cpu(buf[0]); > if (p->policyvers < POLICYDB_VERSION_MIN || > p->policyvers > POLICYDB_VERSION_MAX) { > printk(KERN_ERR "SELinux: policydb version %d does not match " > "my version range %d-%d\n", > le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX); > + rc = -EINVAL; > goto bad; > } > > if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) { > p->mls_enabled = 1; > > - rc = -EINVAL; > if (p->policyvers < POLICYDB_VERSION_MLS) { > printk(KERN_ERR "SELinux: security policydb version %d " > "(MLS) not backwards compatible\n", > p->policyvers); > + rc = -EINVAL; > goto bad; > } > } > @@ -2332,21 +2333,21 @@ int policydb_read(struct policydb *p, void *fp) > goto bad; > } > > - rc = -EINVAL; > info = policydb_lookup_compat(p->policyvers); > if (!info) { > printk(KERN_ERR "SELinux: unable to find policy compat info " > "for version %d\n", p->policyvers); > + rc = -EINVAL; > goto bad; > } > > - rc = -EINVAL; > if (le32_to_cpu(buf[2]) != info->sym_num || > le32_to_cpu(buf[3]) != info->ocon_num) { > printk(KERN_ERR "SELinux: policydb table sizes (%d,%d) do " > "not match mine (%d,%d)\n", le32_to_cpu(buf[2]), > le32_to_cpu(buf[3]), > info->sym_num, info->ocon_num); > + rc = -EINVAL; > goto bad; > } > > @@ -2365,10 +2366,11 @@ int policydb_read(struct policydb *p, void *fp) > p->symtab[i].nprim = nprim; > } > > - rc = -EINVAL; > p->process_class = string_to_security_class(p, "process"); > - if (!p->process_class) > + if (!p->process_class) { > + rc = -EINVAL; > goto bad; > + } > > rc = avtab_read(&p->te_avtab, fp, p); > if (rc) > @@ -2386,10 +2388,12 @@ int policydb_read(struct policydb *p, void *fp) > nel = le32_to_cpu(buf[0]); > ltr = NULL; > for (i = 0; i < nel; i++) { > - rc = -ENOMEM; > tr = kzalloc(sizeof(*tr), GFP_KERNEL); > - if (!tr) > + if (!tr) { > + rc = -ENOMEM; > goto bad; > + } > + > if (ltr) > ltr->next = tr; > else > @@ -2398,7 +2402,6 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -EINVAL; > tr->role = le32_to_cpu(buf[0]); > tr->type = le32_to_cpu(buf[1]); > tr->new_role = le32_to_cpu(buf[2]); > @@ -2410,12 +2413,14 @@ int policydb_read(struct policydb *p, void *fp) > } else > tr->tclass = p->process_class; > > - rc = -EINVAL; > if (!policydb_role_isvalid(p, tr->role) || > !policydb_type_isvalid(p, tr->type) || > !policydb_class_isvalid(p, tr->tclass) || > - !policydb_role_isvalid(p, tr->new_role)) > + !policydb_role_isvalid(p, tr->new_role)) { > + rc = -EINVAL; > goto bad; > + } > + > ltr = tr; > } > > @@ -2425,10 +2430,12 @@ int policydb_read(struct policydb *p, void *fp) > nel = le32_to_cpu(buf[0]); > lra = NULL; > for (i = 0; i < nel; i++) { > - rc = -ENOMEM; > ra = kzalloc(sizeof(*ra), GFP_KERNEL); > - if (!ra) > + if (!ra) { > + rc = -ENOMEM; > goto bad; > + } > + > if (lra) > lra->next = ra; > else > @@ -2437,12 +2444,14 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -EINVAL; > ra->role = le32_to_cpu(buf[0]); > ra->new_role = le32_to_cpu(buf[1]); > if (!policydb_role_isvalid(p, ra->role) || > - !policydb_role_isvalid(p, ra->new_role)) > + !policydb_role_isvalid(p, ra->new_role)) { > + rc = -EINVAL; > goto bad; > + } > + > lra = ra; > } > > @@ -2454,11 +2463,12 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -EINVAL; > p->process_trans_perms = string_to_av_perm(p, p->process_class, "transition"); > p->process_trans_perms |= string_to_av_perm(p, p->process_class, "dyntransition"); > - if (!p->process_trans_perms) > + if (!p->process_trans_perms) { > + rc = -EINVAL; > goto bad; > + } > > rc = ocontext_read(p, info, fp); > if (rc) > @@ -2472,12 +2482,13 @@ int policydb_read(struct policydb *p, void *fp) > if (rc) > goto bad; > > - rc = -ENOMEM; > p->type_attr_map_array = flex_array_alloc(sizeof(struct ebitmap), > p->p_types.nprim, > GFP_KERNEL | __GFP_ZERO); > - if (!p->type_attr_map_array) > + if (!p->type_attr_map_array) { > + rc = -ENOMEM; > goto bad; > + } > > /* preallocate so we don't have to worry about the put ever failing */ > rc = flex_array_prealloc(p->type_attr_map_array, 0, p->p_types.nprim, > -- > 2.11.0 >
>> One local variable was set to an error code in some cases before >> a concrete error situation was detected. Thus move the corresponding >> assignments into if branches to indicate a software failure there. >> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >> --- >> security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++----------------- >> 1 file changed, 35 insertions(+), 24 deletions(-) > > More code churn with no real advantage. There are different opinions about the mentioned implementation details. > I agree with the style you are using, Thanks for such feedback. > and would support changing it if you are in the function fixing bugs > or doing other substantial changes in that code, Is this expectation a contradiction for a desired patch granularity? > but I can't justify it as a standalone change, sorry. This update suggestion seems to be not attractive enough for you at the moment as another change step of my patch series. Would you like to check if there are other effects worthwhile besides the proposed coding style adjustment here? Regards, Markus
On Fri, Mar 24, 2017 at 6:09 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> One local variable was set to an error code in some cases before >>> a concrete error situation was detected. Thus move the corresponding >>> assignments into if branches to indicate a software failure there. >>> >>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >>> --- >>> security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++----------------- >>> 1 file changed, 35 insertions(+), 24 deletions(-) >> >> More code churn with no real advantage. > > There are different opinions about the mentioned implementation details. > >> I agree with the style you are using, > > Thanks for such feedback. > >> and would support changing it if you are in the function fixing bugs >> or doing other substantial changes in that code, > > Is this expectation a contradiction for a desired patch granularity? > >> but I can't justify it as a standalone change, sorry. > > This update suggestion seems to be not attractive enough for you at the moment > as another change step of my patch series. > Would you like to check if there are other effects worthwhile besides the proposed > coding style adjustment here? To be honest, I would just leave it alone for now. If you want to contribute, focus on meaningful improvements such as bug fixing and/or new features; changing only the code style isn't very interesting or appealing, even if I happen to agree with your changes.
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 53e6d06e772a..506b0228d1f1 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -2250,15 +2250,14 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -EINVAL; if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) { printk(KERN_ERR "SELinux: policydb magic number 0x%x does " "not match expected magic number 0x%x\n", le32_to_cpu(buf[0]), POLICYDB_MAGIC); + rc = -EINVAL; goto bad; } - rc = -EINVAL; len = le32_to_cpu(buf[1]); if (len != strlen(POLICYDB_STRING)) { printk(KERN_ERR "SELinux: policydb string length %d does not " @@ -2265,11 +2265,13 @@ int policydb_read(struct policydb *p, void *fp) len, strlen(POLICYDB_STRING)); + rc = -EINVAL; goto bad; } - rc = -ENOMEM; policydb_str = kmalloc(len + 1, GFP_KERNEL); - if (!policydb_str) + if (!policydb_str) { + rc = -ENOMEM; goto bad; + } rc = next_entry(policydb_str, fp, len); if (rc) { @@ -2279,12 +2280,12 @@ int policydb_read(struct policydb *p, void *fp) goto bad; } - rc = -EINVAL; policydb_str[len] = '\0'; if (strcmp(policydb_str, POLICYDB_STRING)) { printk(KERN_ERR "SELinux: policydb string %s does not match " "my string %s\n", policydb_str, POLICYDB_STRING); kfree(policydb_str); + rc = -EINVAL; goto bad; } /* Done with policydb_str. */ @@ -2296,24 +2297,24 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -EINVAL; p->policyvers = le32_to_cpu(buf[0]); if (p->policyvers < POLICYDB_VERSION_MIN || p->policyvers > POLICYDB_VERSION_MAX) { printk(KERN_ERR "SELinux: policydb version %d does not match " "my version range %d-%d\n", le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX); + rc = -EINVAL; goto bad; } if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) { p->mls_enabled = 1; - rc = -EINVAL; if (p->policyvers < POLICYDB_VERSION_MLS) { printk(KERN_ERR "SELinux: security policydb version %d " "(MLS) not backwards compatible\n", p->policyvers); + rc = -EINVAL; goto bad; } } @@ -2332,21 +2333,21 @@ int policydb_read(struct policydb *p, void *fp) goto bad; } - rc = -EINVAL; info = policydb_lookup_compat(p->policyvers); if (!info) { printk(KERN_ERR "SELinux: unable to find policy compat info " "for version %d\n", p->policyvers); + rc = -EINVAL; goto bad; } - rc = -EINVAL; if (le32_to_cpu(buf[2]) != info->sym_num || le32_to_cpu(buf[3]) != info->ocon_num) { printk(KERN_ERR "SELinux: policydb table sizes (%d,%d) do " "not match mine (%d,%d)\n", le32_to_cpu(buf[2]), le32_to_cpu(buf[3]), info->sym_num, info->ocon_num); + rc = -EINVAL; goto bad; } @@ -2365,10 +2366,11 @@ int policydb_read(struct policydb *p, void *fp) p->symtab[i].nprim = nprim; } - rc = -EINVAL; p->process_class = string_to_security_class(p, "process"); - if (!p->process_class) + if (!p->process_class) { + rc = -EINVAL; goto bad; + } rc = avtab_read(&p->te_avtab, fp, p); if (rc) @@ -2386,10 +2388,12 @@ int policydb_read(struct policydb *p, void *fp) nel = le32_to_cpu(buf[0]); ltr = NULL; for (i = 0; i < nel; i++) { - rc = -ENOMEM; tr = kzalloc(sizeof(*tr), GFP_KERNEL); - if (!tr) + if (!tr) { + rc = -ENOMEM; goto bad; + } + if (ltr) ltr->next = tr; else @@ -2398,7 +2402,6 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -EINVAL; tr->role = le32_to_cpu(buf[0]); tr->type = le32_to_cpu(buf[1]); tr->new_role = le32_to_cpu(buf[2]); @@ -2410,12 +2413,14 @@ int policydb_read(struct policydb *p, void *fp) } else tr->tclass = p->process_class; - rc = -EINVAL; if (!policydb_role_isvalid(p, tr->role) || !policydb_type_isvalid(p, tr->type) || !policydb_class_isvalid(p, tr->tclass) || - !policydb_role_isvalid(p, tr->new_role)) + !policydb_role_isvalid(p, tr->new_role)) { + rc = -EINVAL; goto bad; + } + ltr = tr; } @@ -2425,10 +2430,12 @@ int policydb_read(struct policydb *p, void *fp) nel = le32_to_cpu(buf[0]); lra = NULL; for (i = 0; i < nel; i++) { - rc = -ENOMEM; ra = kzalloc(sizeof(*ra), GFP_KERNEL); - if (!ra) + if (!ra) { + rc = -ENOMEM; goto bad; + } + if (lra) lra->next = ra; else @@ -2437,12 +2444,14 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -EINVAL; ra->role = le32_to_cpu(buf[0]); ra->new_role = le32_to_cpu(buf[1]); if (!policydb_role_isvalid(p, ra->role) || - !policydb_role_isvalid(p, ra->new_role)) + !policydb_role_isvalid(p, ra->new_role)) { + rc = -EINVAL; goto bad; + } + lra = ra; } @@ -2454,11 +2463,12 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -EINVAL; p->process_trans_perms = string_to_av_perm(p, p->process_class, "transition"); p->process_trans_perms |= string_to_av_perm(p, p->process_class, "dyntransition"); - if (!p->process_trans_perms) + if (!p->process_trans_perms) { + rc = -EINVAL; goto bad; + } rc = ocontext_read(p, info, fp); if (rc) @@ -2472,12 +2482,13 @@ int policydb_read(struct policydb *p, void *fp) if (rc) goto bad; - rc = -ENOMEM; p->type_attr_map_array = flex_array_alloc(sizeof(struct ebitmap), p->p_types.nprim, GFP_KERNEL | __GFP_ZERO); - if (!p->type_attr_map_array) + if (!p->type_attr_map_array) { + rc = -ENOMEM; goto bad; + } /* preallocate so we don't have to worry about the put ever failing */ rc = flex_array_prealloc(p->type_attr_map_array, 0, p->p_types.nprim,