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

[Tyler Hicks]

On 2016-03-14 21:09:07, Seth Arnold wrote:
> On Mon, Mar 14, 2016 at 10:39:09PM -0500, Tyler Hicks wrote:
> > -		yyerror(_("Unknown error while parsing label: %s\n"), label);
> 
> > +			yyerror(_(err), label);
> 
> Does this change break the translator tools that gather strings to
> translate? (It's a nice cleanup otherwise.)

It does. Nice catch.

I've identified a small change to fix it.

> 
> >  	if (!*_name) {
> >  		free(*_ns);
> > -		yyerror(_("Memory allocation error."));
> > +		goto alloc_fail;
> >  	}
> > +
> > +	return true;
> > +
> > +alloc_fail:
> > +	err = "Memory allocation error.";
> > +	if (yyerr)
> > +		yyerror(_(err));
> > +	else
> > +		fprintf(stderr, "%s", _(err));
> > +
> > +	return false;
> 
> This appears to be a behaviour change beyond a cleanup -- previously it'd
> call yyerror() then return true. I suspect this is intentional but I
> wanted to call it out as more than a cleanup...

yyerror() actually calls exit() so we're not changing behavior here:

void yyerror(const char *msg, ...)
{
        va_list arg;

        va_start(arg, msg);
        vprintyyerror(msg, arg);
        va_end(arg);

        exit(1);
}

> 
> > +		if (!kernel_supports_stacking) {
> > +			bool stack;
> > +
> > +			if (!parse_label(&stack, &ns, &name,
> > +					 tbuf.c_str(), false)) {
> > +				return FALSE;
> > +			} else if (stack) {
> 
> I'm not a fan of this construct:
> 
> if (set_variable(&foo))
>   /* branch A */
> else if (foo)
>   /* branch B */
> 
> This actually makes branch B dependant upon both the return value of the
> function _and_ the variable but that's not obvious as the two are
> separated by some distance. (It's not much, here, but it's enough to be
> confusing.)

Do you prefer this instead?

if (set_variable(&foo))
  /* branch A */

if (foo)
  /* branch B */

Tyler

> 
> There's four cases of return-value and stack and it's not clear on first
> glance how many of them are handled explicitly.
> 
> > +					_("The current kernel does not support stacking of named transitions: %s\n"),
> > +					tbuf.c_str());
> > +				return FALSE;
> > +			}
> 
> 
> Thanks



> -- 
> 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/20160315/02ec3f81/attachment.pgp>