[apparmor] [PATCH 4/6] parser: stop splitting the namespace from the named transition targets

[Tyler Hicks]

On 2016-03-15 22:13:28, Seth Arnold wrote:
> On Tue, Mar 15, 2016 at 10:38:25PM -0500, Tyler Hicks wrote:
> > > Do you prefer this instead?
> > > 
> > > if (set_variable(&foo))
> > >   /* branch A */
> > > 
> > > if (foo)
> > >   /* branch B */
> 
> I do; it's not identical in the general case but it is in this case and
> it's a lot easier for me to read. Thanks.

No problem.

> 
> > -void parse_label(bool *_stack, char **_ns, char **_name, const char *label)
> > +bool parse_label(bool *_stack, char **_ns, char **_name,
> > +		 const char *label, bool yyerr)
> >  {
> 
> > +	if (err) {
> > +		if (yyerr)
> > +			yyerror(err, label);
> > +		else
> > +			fprintf(stderr, err, label);
> > +
> > +		return false;
> >  	}
> 
> Just a passing fancy that if instead of "bool yyerr" the parameter were
> "(void)(yyerr(char*, char*))" (or something along those lines) then we could
> supply function pointers to describe how to handle the error, and remove
> half the code in this block, which we'll probably wind up duplicating into
> more and more functions.
> 
> Then callers would call e.g.:
> 
> parse_label(&stack, &ns, &name, label, &yyerror);
> 
> or
> 
> void errors_to_stderr(const char *err, const char *label)
> {
> 	fprintf(stderr, err, label);
> }
> parse_label(&stack, &ns, &name, label, &errors_to_stderr);
> 
> Maybe tonight's not the night to sort out our error handling but including
> blocks like this everywhere would probably get tiring.

I thought about something like that but since some messages don't
include the label string and some do, it makes it a little messy.

> 
> > @@ -585,7 +587,27 @@ static int process_dfa_entry(aare_rules *dfarules, struct cod_entry *entry)
> >  			/* allow change_profile for all execs */
> >  			vec[0] = "/[^/\\x00][^\\x00]*";
> >  
> > -		vec[index++] = tbuf.c_str();
> > +		if (!kernel_supports_stacking) {
> > +			bool stack;
> > +
> > +			if (!parse_label(&stack, &ns, &name,
> > +					 tbuf.c_str(), false)) {
> > +				return FALSE;
> > +			}
> > +
> > +			if (stack) {
> > +				fprintf(stderr,
> > +					_("The current kernel does not support stacking of named transitions: %s\n"),
> > +					tbuf.c_str());
> > +				return FALSE;
> > +			}
> 
> Thanks, this is easier for me to follow.
> 
> Another aside, there's a similar error message for audit messages in this
> function that's not labeled for translation. We may want to set aside some
> time to make a pass through the parser and add or remove messages from
> translations.

I have noticed those while going through various parts of the code. I'll
start trying to do a better job of fixing those issues as I see them.

> 
> Acked-by: Seth Arnold <seth.arnold at canonical.com>
> 
> Thanks

Thank you!

Tyler



> -- 
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160318/657d9766/attachment.pgp>