Re: [PATCH net-next 3/4 v4] amt: add multicast(IGMP) report message handler

[Taehee Yoo <ap420073@gmail.com>]

Hi David,
Thank you for your review!

On 10/28/21 12:08 AM, David Ahern wrote:
 > On 10/26/21 9:10 AM, Taehee Yoo wrote:
 >> +static bool amt_status_filter(struct amt_source_node *snode,
 >> +			      enum amt_filter filter)
 >> +{
 >
 > How about:
 > 	bool rc = false;
 >
 > and then
 >
 >> +	switch (filter) {
 >> +	case AMT_FILTER_FWD:
 >> +		if (snode->status == AMT_SOURCE_STATUS_FWD &&
 >> +		    snode->flags == AMT_SOURCE_OLD)
 >> +			rc = true;
 >> +		break;
 > similar change for the rest of the cases.
 >

Thanks, I will use it.

 >> +	case AMT_FILTER_D_FWD:
 >> +		if (snode->status == AMT_SOURCE_STATUS_D_FWD &&
 >> +		    snode->flags == AMT_SOURCE_OLD)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	case AMT_FILTER_FWD_NEW:
 >> +		if (snode->status == AMT_SOURCE_STATUS_FWD &&
 >> +		    snode->flags == AMT_SOURCE_NEW)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	case AMT_FILTER_D_FWD_NEW:
 >> +		if (snode->status == AMT_SOURCE_STATUS_D_FWD &&
 >> +		    snode->flags == AMT_SOURCE_NEW)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	case AMT_FILTER_ALL:
 >> +		return true;
 >> +	case AMT_FILTER_NONE_NEW:
 >> +		if (snode->status == AMT_SOURCE_STATUS_NONE &&
 >> +		    snode->flags == AMT_SOURCE_NEW)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	case AMT_FILTER_BOTH:
 >> +		if ((snode->status == AMT_SOURCE_STATUS_D_FWD ||
 >> +		     snode->status == AMT_SOURCE_STATUS_FWD) &&
 >> +		    snode->flags == AMT_SOURCE_OLD)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	case AMT_FILTER_BOTH_NEW:
 >> +		if ((snode->status == AMT_SOURCE_STATUS_D_FWD ||
 >> +		     snode->status == AMT_SOURCE_STATUS_FWD) &&
 >> +		    snode->flags == AMT_SOURCE_NEW)
 >> +			return true;
 >> +		else
 >> +			return false;
 >> +	default:
 >> +		return false;
 >> +	}
 >> +
 >> +	return false;
 >> +}
 >> +
 >
 >
 >> +
 >> +/* If a source timer expires with a router filter-mode for the group of
 >> + * INCLUDE, the router concludes that traffic from this particular
 >> + * source is no longer desired on the attached network, and deletes the
 >> + * associated source record.
 >> + */
 >> +static void amt_source_work(struct work_struct *work)
 >> +{
 >> +	struct amt_source_node *snode = container_of(to_delayed_work(work),
 >> +						     struct amt_source_node,
 >> +						     source_timer);
 >> +	struct amt_group_node *gnode = snode->gnode;
 >> +	struct amt_dev *amt = gnode->amt;
 >> +	struct amt_tunnel_list *tunnel;
 >> +
 >> +	tunnel = gnode->tunnel_list;
 >> +	spin_lock_bh(&tunnel->lock);
 >> +	rcu_read_lock();
 >> +	if (gnode->filter_mode == MCAST_INCLUDE) {
 >> +		amt_destroy_source(snode);
 >> +		if (!gnode->nr_sources)
 >> +			amt_del_group(amt, gnode);
 >> +	} else {
 >> +/* When a router filter-mode for a group is EXCLUDE, source records are
 >> + * only deleted when the group timer expires
 >> + */
 >
 > comment needs to be indented.
 >

Okay, I will fix it.

 >> +		snode->status = AMT_SOURCE_STATUS_D_FWD;
 >> +	}
 >> +	rcu_read_unlock();
 >> +	spin_unlock_bh(&tunnel->lock);
 >> +}
 >> +
 >
 >

Thanks a lot for your reviews.
I will fix these things and send v2 patch.

Thanks,
Taehee