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
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: PGP signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
