[apparmor] [PATCH 3/3] Add mount rules

[James Troup]

John Johansen <john.johansen at canonical.com> writes:

a) What is it with you and Kees and not trimming replies? :-P

b) Most of my comments are not substantive and limited to trivial issues
   as I'm only reading the patch for educational purposes and not really
   qualified to review the code.  If it's not of interest, I'm happy to
   do any read further in silence, just let me know.

> +++ b/parser/mount.c

[...]

> + *   Copyright (c) 2010
> + *   Canonical, Ltd. (All rights reserved)

As Kees said, 2012 - but also s/Canonical, Ltd./Canonical Ltd./

Same for all files. 

> + * -F fork for similtaneous mount

s/similtaneous/simultaneous/

> + * The linux kernel has 32 fs - independent mount flags, that mount command

s/linux/Linux/

> + * is responsible for stripping out and mapping to a 32 bit flags field.
> + * The mount commands mapping is documented below.
> + *
> + * Unfortunately we can not directly use this mapping as we need to be able
> + * represent, whether none, 1 or both options of a flag can be present for
> + *

for ... what?

> + * eg.

s/eg./e.g./

> + *    ro, and rw information is stored in a single bit.  But we need 2 bits
> + *    of information.
> + *    ro - the mount can only be readonly
> + *    rw - the mount can only be rw
> + *    ro/rw - the mount can be either ro/rw
> + *    the forth state of neither ro/rw does not exist, but still we need

s/forth/fourth/ (presumably?)

> + * Note: leading owner option applies owner condition to both sours and dest

s/sours/source/

> + * If a condition is not specified then it is assumed to match all possible
> + * entries for it.  ie. a missing fstype means all fstypes are matced.

s/ie./i.e./, s/matced/matched/

> + * However if a condition is specified then the rule only grants permission
> + * for mounts matching the specified pattern.
> + *
> + * Egs.

Perhaps: 'Examples:' or just straight up 'e.g.'

> + *   to catch mount options, that can't be covered.

s/, that/that/

> +			PDEBUG("  extracting fstype ");

Missing \n ?

> +	ent = (struct mnt_entry *) calloc(1, sizeof(struct mnt_entry));
> +	if (ent) {

Maybe this is simply project style and/or a personal preference, but
wouldn't it be cleaner to do:

  ent = (struct mnt_entry *) calloc(1, sizeof(struct mnt_entry));
    if (!ent)
      return ent

Which would allow the rest of the function to be deindented?

> +		ent->dev_type = extract_fstype(&sconds);

It may just be me, but my brain automatically expands that to 'seconds'.
It might be nice to use src_conds and dst_conds like you did elsewhere
here too.

> +		fprintf(stderr, "error: unknonw mount perm");

s/unknonw/unknown/

> +++ b/parser/mount.h


> +#define AA_MAY_REMOUNT 8	/* dump perm for remount rule */

What does this comment mean?

> +++ b/parser/parser_policy.c

> +				PERROR("Profile %s has to many specified profile transitions.\n", cod->name);

s/to/too/

> +++ b/parser/parser_regex.c

> +		if (ent->next && size - (p -buffer) > 1) {

s/-buffer/- buffer/

> +		/* remount is can't be conditional on device and type */

s/is //

> +		pwarn("profile %s mount rules not enforce\n", cod->name);

not enforced?  not enforceable?

> +++ b/parser/parser_variable.c

> +			if (!dup_and_chain(entry)) {
>  				PERROR("Memory allocaton error while handling set variable %s\n",

It's pre-existing, but: s/allocaton/allocation/

>  static int process_variables_in_entries(struct cod_entry *entry_list)
>  {
>  	int ret = TRUE, rc;
>  	struct cod_entry *entry;
>  
>  	list_for_each(entry_list, entry) {
> -		rc = expand_entry_variables(entry);
> +		rc = expand_entry_variables(&entry->name, entry,
> +					    clone_and_chain_cod);
>  		if (!rc)
> -			ret = FALSE;
> +			return FALSE;
> +	}
> +
> +	return ret;
> +}
>

Couldn't you just 'return TRUE;' here and drop the 'ret' variable altogether?

> +static int process_variables_in_mnt_entries(struct mnt_entry *entry_list)

[...]

>  	return ret;

Same here.

> +++ b/parser/parser_yacc.y

> +		//		val->next = $1;

?

-- 
James