On Thu, 22 Apr 2010, Eric Paris wrote:
> policydb.c has lots of different standards on how to handle return paths on
> error. For the most part transition to
>
> rc=errno
> if (failure)
> goto out;
> [...]
> out:
> cleanup()
> return rc;
>
> Instead of doing cleanup mid function, or having multiple returns or other
> options. This doesn't do that for every function, but most of the complex
> functions which have cleanup routines on error.
I think the idea is that the 'goto out' paradigm is used when multiple
return points need the same cleanup, otherwise, just use return normally.
> +++ b/security/selinux/ss/policydb.c
> @@ -144,35 +144,33 @@ static int roles_init(struct policydb *p)
> {
> char *key = NULL;
> int rc;
> - struct role_datum *role;
> + struct role_datum *role = NULL;
Why is this being initialized to NULL? The kzalloc call will assign it a
definite value. Initializing variables unnecessarily can hide bugs in the
code.
> + rc = -ENOMEM;
> role = kzalloc(sizeof(*role), GFP_KERNEL);
I'm not sure what the point is of assigning the error code first --
perhaps if all or most of the error cases use that error, but they don't.
> -out:
> - return rc;
> + return 0;
>
> out_free_symtab:
> for (i = 0; i < SYM_NUM; i++)
> hashtab_destroy(p->symtab[i].table);
> - goto out;
> + return rc;
> }
Also, I think part of the point of this approach is that you reduce, not
increase, the number of return statements, to make reading the code
simpler. It also forces you to account for 'rc' properly at all times.
- James
--
James Morris
<jmorris@xxxxxxxxx>
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
