netdev - Re: [PATCH net-next v2 01/13] net: sched: Pass qdisc reference in struct flow_block

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k0z7fxj3.fsf@mellanox.com>
Date:   Mon, 13 Jul 2020 13:46:08 +0200
From:   Petr Machata <petrm@...lanox.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, Pablo Neira Ayuso <pablo@...filter.org>,
        davem@...emloft.net, jiri@...lanox.com, mlxsw@...lanox.com,
        michael.chan@...adcom.com, saeedm@...lanox.com, leon@...nel.org,
        kadlec@...filter.org, fw@...len.de, jhs@...atatu.com,
        xiyou.wangcong@...il.com, simon.horman@...ronome.com,
        Ido Schimmel <idosch@...lanox.com>
Subject: Re: [PATCH net-next v2 01/13] net: sched: Pass qdisc reference in struct flow_block_offload

Jakub Kicinski <kuba@...nel.org> writes:

> On Sat, 11 Jul 2020 00:55:03 +0300 Petr Machata wrote:
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>> index 0a9a4467d7c7..e82e5cf64d61 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c

>> @@ -1911,7 +1911,7 @@ static int bnxt_tc_setup_indr_block(struct net_device *netdev, struct bnxt *bp,
>>  		block_cb = flow_indr_block_cb_alloc(bnxt_tc_setup_indr_block_cb,
>>  						    cb_priv, cb_priv,
>>  						    bnxt_tc_setup_indr_rel, f,
>> -						    netdev, data, bp, cleanup);
>> +						    netdev, sch, data, bp, cleanup);
>
> nit: the number of arguments is getting out of hand here, perhaps it's
>      time to pass a structure in.

I would say that adding a distinctly-typed and distinctly-named argument
does not make the other arguments less clear, or increase the risk of
mixing them up in a way that the compiler would not catch.

Taken from another angle, the argument list is not passed through layers
of other functions, so the structure would be assembled in
bnxt_tc_setup_indr_block() and then unpacked again in
flow_indr_block_cb_alloc(). Nothing is saved in that regard either.

Also, flow_indr_block_cb_alloc() is a simple function. When a structure
needs to be introduced to carry the arguments, it might be more
straightforward to just open-code it.

>>  		if (IS_ERR(block_cb)) {
>>  			list_del(&cb_priv->list);
>>  			kfree(cb_priv);