Re: [PATCH v2 net-next 2/5] ipv4: Add fib_nh_common to fib_result

[David Ahern <dsahern@gmail.com>]

On 4/1/19 12:20 PM, Martin Lau wrote:
>> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
>> index 12a6d759cf57..5070bc531ca4 100644
>> --- a/include/net/ip_fib.h
>> +++ b/include/net/ip_fib.h
>> @@ -156,15 +156,16 @@ struct fib_rule;
>>  
>>  struct fib_table;
>>  struct fib_result {
>> -	__be32		prefix;
>> -	unsigned char	prefixlen;
>> -	unsigned char	nh_sel;
>> -	unsigned char	type;
>> -	unsigned char	scope;
>> -	u32		tclassid;
>> -	struct fib_info *fi;
>> -	struct fib_table *table;
>> -	struct hlist_head *fa_head;
>> +	__be32			prefix;
>> +	unsigned char		prefixlen;
>> +	unsigned char		nh_sel;
> Is nh_sel still needed?

yes, for fib_result_nl API and nl_fib_lookup.


>> diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
>> index df777af7e278..f81c7dc7ff59 100644
>> --- a/net/ipv4/fib_semantics.c
>> +++ b/net/ipv4/fib_semantics.c
>> @@ -1075,6 +1075,24 @@ __be32 fib_info_update_nh_saddr(struct net *net, struct fib_nh *nh)
>>  	return nh->nh_saddr;
>>  }
>>  
>> +__be32 fib_result_prefsrc(struct net *net, struct fib_result *res)
>> +{
>> +	struct fib_nh_common *nhc = res->nhc;
>> +	struct fib_nh *nh;
>> +
>> +	if (res->fi->fib_prefsrc)
>> +		return res->fi->fib_prefsrc;
>> +
>> +	if (unlikely(nhc->nhc_family != AF_INET))
> Comparing with FIB_RES_PREFSRC, it is not immediately obvious
> why this test is needed.  May be a comment or a commit log message?

This branch is for the case of v6 with v4. I can remove from this patch
and add later. Right now, nhc will only be from a fib_nh.

> 
>> +		return inet_select_addr(nhc->nhc_dev, 0, res->fi->fib_scope);
>> +
>> +	nh = container_of(nhc, struct fib_nh, nh_common);
>> +	if (nh->nh_saddr_genid == atomic_read(&net->ipv4.dev_addr_genid))
>> +		return nh->nh_saddr;
>> +
>> +	return fib_info_update_nh_saddr(net, nh);
>> +}
>> +
>>  static bool fib_valid_prefsrc(struct fib_config *cfg, __be32 fib_prefsrc)
>>  {
>>  	if (cfg->fc_type != RTN_LOCAL || !cfg->fc_dst ||
>> @@ -1762,20 +1780,22 @@ void fib_select_multipath(struct fib_result *res, int hash)
>>  	struct net *net = fi->fib_net;
>>  	bool first = false;
>>  
>> -	for_nexthops(fi) {
>> +	change_nexthops(fi) {
> Is it mainly because of the 'const'?

yes.