| Message ID | dbd8e89d-45a1-5785-f2dd-673389ac01a3@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 1/15/2017 7:15 AM, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 14 Jan 2017 18:29:20 +0100 > > Adjust a jump target to avoid a check repetition at the end after a memory > allocation failed for the local variable "newgenfs". > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > security/selinux/ss/policydb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index 5dc31faa601f..e7b882251da8 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp) > newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL); > if (!newgenfs) { > rc = -ENOMEM; > - goto out; > + goto exit; > } > > rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); > @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp) > kfree(newgenfs); > } > ocontext_destroy(newc, OCON_FSUSE); > - > +exit: > return rc; Why not replace the "goto out" with "return rc" rather than add a target? > } >
>> @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp) >> newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL); >> if (!newgenfs) { >> rc = -ENOMEM; >> - goto out; >> + goto exit; >> } >> >> rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); >> @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp) >> kfree(newgenfs); >> } >> ocontext_destroy(newc, OCON_FSUSE); >> - >> +exit: >> return rc; > > Why not replace the "goto out" with "return rc" rather > than add a target? Would you accept to use the statement "return -ENOMEM;" there instead? Regards, Markus
On 1/17/2017 8:37 AM, SF Markus Elfring wrote: >>> @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp) >>> newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL); >>> if (!newgenfs) { >>> rc = -ENOMEM; >>> - goto out; >>> + goto exit; >>> } >>> >>> rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); >>> @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp) >>> kfree(newgenfs); >>> } >>> ocontext_destroy(newc, OCON_FSUSE); >>> - >>> +exit: >>> return rc; >> Why not replace the "goto out" with "return rc" rather >> than add a target? > Would you accept to use the statement "return -ENOMEM;" there instead? That would be even better. > > Regards, > Markus >
On Tue, Jan 17, 2017 at 12:53 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 1/17/2017 8:37 AM, SF Markus Elfring wrote: >>>> @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp) >>>> newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL); >>>> if (!newgenfs) { >>>> rc = -ENOMEM; >>>> - goto out; >>>> + goto exit; >>>> } >>>> >>>> rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); >>>> @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp) >>>> kfree(newgenfs); >>>> } >>>> ocontext_destroy(newc, OCON_FSUSE); >>>> - >>>> +exit: >>>> return rc; >>> Why not replace the "goto out" with "return rc" rather >>> than add a target? >> Would you accept to use the statement "return -ENOMEM;" there instead? > > That would be even better. I *hate* code that does a jump to a label only to then do a return/exit. That said, see my earlier comments about not worrying too much about performance of the error path and in this case I like the "feel good" nature of the code where ever failure in the loop goes to "out".
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 5dc31faa601f..e7b882251da8 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp) newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL); if (!newgenfs) { rc = -ENOMEM; - goto out; + goto exit; } rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp) kfree(newgenfs); } ocontext_destroy(newc, OCON_FSUSE); - +exit: return rc; }