Re: [PATCH RFC net-next 08/14] bpf: add eBPF verifier

[Alexei Starovoitov]

On Wed, Jul 2, 2014 at 1:11 AM, David Laight <David.Laight@xxxxxxxxxx> wrote:
> From: Alexei Starovoitov
> ...
>> >> +#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })
> ...
>> >> +       _(get_map_info(env, map_id, &map));
>> >
>> > Nit: such macros should be removed, please.
>>
>> It may surely look unconventional, but alternative is to replace
>> every usage of _ macro with:
>> err =
>> if (err)
>>   return err;
>>
>> and since this macro is used 38 times, it will add ~120 unnecessary
>> lines that will only make code much harder to follow.
>> I tried not using macro and results were not pleasing.
>
> The problem is that they are hidden control flow.
> As such they make flow analysis harder for the casual reader.

In the abstract context macros with gotos and returns are bad,
but in this case extra verbosity is the bigger evil.

Consider this piece of code:
#define _(OP) ({ int ret = OP; if (ret < 0) return ret; })

if (opcode == BPF_END || opcode == BPF_NEG) {
    if (BPF_SRC(insn->code) != BPF_X)
        return -EINVAL;

    /* check src operand */
     _(check_reg_arg(regs, insn->dst_reg, 1));

    /* check dest operand */
    _(check_reg_arg(regs, insn->dst_reg, 0));

} else if (opcode == BPF_MOV) {

    if (BPF_SRC(insn->code) == BPF_X)
        /* check src operand */
        _(check_reg_arg(regs, insn->src_reg, 1));

    /* check dest operand */
    _(check_reg_arg(regs, insn->dst_reg, 0));

where casual reader can easily see what the purpose of the code
and what it's doing.

Now rewrite it without '_' macro:
if (opcode == BPF_END || opcode == BPF_NEG) {
    if (BPF_SRC(insn->code) != BPF_X)
            return -EINVAL;

    /* check src operand */
    err = check_reg_arg(regs, insn->dst_reg, 1);
    if (err)
        return err;

    /* check dest operand */
    err = check_reg_arg(regs, insn->dst_reg, 0);
    if (err)
        return err;

} else if (opcode == BPF_MOV) {

    if (BPF_SRC(insn->code) == BPF_X) {
        /* check src operand */
        err = check_reg_arg(regs, insn->src_reg, 1);
        if (err)
            return err;
    }

    /* check dest operand */
    err = check_reg_arg(regs, insn->dst_reg, 0);
    if (err)
        return err;

see how your eyes are now picking up endless control flow of
if conditions and returns, instead of focusing on the code itself.
It's much easier to understand the semantics when if (err) is out
of the way. Note that replacing _ with real name will ruin
the reading experience, since CAPITAL letters of the macro
will be screaming: "look at me", instead of letting reviewer
focus on the code.

I believe that this usage of _ as a macro specifically as defined,
would be a great addition to kernel coding style in general.
I don't want to see _ to be redefined differently.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/