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.)

>       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...

> +             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.)

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

Attachment: signature.asc
Description: Digital signature

-- 
AppArmor mailing list
[email protected]
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to