netdev - Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation inside DSA

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]

Date:   Tue, 11 Jul 2017 11:32:09 +0300
From:   Arkadi Sharshevsky <arkadis@...lanox.com>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, jiri@...nulli.us, ivecera@...hat.com,
        f.fainelli@...il.com, andrew@...n.ch, Woojung.Huh@...rochip.com,
        stephen@...workplumber.org, mlxsw@...lanox.com
Subject: Re: [PATCH net-next RFC 10/12] net: dsa: Move FDB dump implementation
 inside DSA



On 07/10/2017 10:36 PM, Vivien Didelot wrote:
> Hi Arkadi,
> 
> Arkadi Sharshevsky <arkadis@...lanox.com> writes:
> 
>> +struct dsa_slave_dump_ctx {
>> +	struct net_device *dev;
>> +	struct sk_buff *skb;
>> +	struct netlink_callback *cb;
>> +	int idx;
>> +};
>> +
>>  struct dsa_switch_ops {
>>  	/*
>>  	 * Legacy probing.
>> @@ -392,9 +399,7 @@ struct dsa_switch_ops {
>>  	int	(*port_fdb_del)(struct dsa_switch *ds, int port,
>>  				const unsigned char *addr, u16 vid);
>>  	int	(*port_fdb_dump)(struct dsa_switch *ds, int port,
>> -				 struct switchdev_obj_port_fdb *fdb,
>> -				  switchdev_obj_dump_cb_t *cb);
>> -
>> +				 struct dsa_slave_dump_ctx *dump);
> 
> I would prefer to pass a callback to the driver, so that we can call
> port_fdb_dump for other interfaces like debugfs, and for non exposed
> switch ports (CPU and DSA links) as well. Something like:
> 
>     typedef int dsa_fdb_dump_cb_t(const unsigned char *addr, u16 vid,
>                                   bool is_static, void *data);
> 
>     int (*port_fdb_dump)(struct dsa_switch *ds, int port,
>                          dsa_fdb_dump_cb_t *cb, void *data);
> 
> Then dsa_slave_dump_ctx and dsa_slave_port_fdb_do_dump can be static to
> net/dsa/slave.c.
> 

Originally thought about it. But it makes sense to pass a callback
when different functions can be supplied.

Here is only one option which is the fdb_do_dump(). So I thought
exporting it makes more sense..

>> +int dsa_slave_port_fdb_dump(struct sk_buff *skb, struct netlink_callback *cb,
>> +			    struct net_device *dev,
>> +			    struct net_device *filter_dev, int *idx)
>> +{
>> +	struct dsa_slave_dump_ctx dump = {
>> +		.dev = dev,
>> +		.skb = skb,
>> +		.cb = cb,
>> +		.idx = *idx,
>> +	};
>> +	struct dsa_slave_priv *p = netdev_priv(dev);
>> +	struct dsa_port *dp = p->dp;
>> +	struct dsa_switch *ds = dp->ds;
>> +	int err;
>> +
>> +	if (!ds->ops->port_fdb_dump) {
>> +		err = -EOPNOTSUPP;
>> +		goto out;
>> +	}
>> +
>> +	err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
>> +out:
>> +	*idx = dump.idx;
>> +	return err;
> 
> There is no need to reassign *idx in case of error:
> 
>     if (!ds->ops->port_fdb_dump)
>         return -EOPNOTSUPP;
> 
>     err = ds->ops->port_fdb_dump(ds, dp->index, &dump);
>     *idx = dump.idx;
> 
>     return err;
>
Will fix, thanks.

>> +	.ndo_fdb_dump		= dsa_slave_port_fdb_dump,
> 
> And s/dsa_slave_port_fdb_dump/dsa_slave_fdb_dump/ here will be even
> better ;-)
> 
> 
> Thanks,
> 
>         Vivien
> 
Will fix, thanks.