Re: [RFC PATCH net-next 10/12] drop_monitor: Add packet alert mode

[Ido Schimmel <idosch@idosch.org>]

On Wed, Jul 24, 2019 at 02:53:41PM +0200, Jiri Pirko wrote:
> Mon, Jul 22, 2019 at 08:31:32PM CEST, idosch@idosch.org wrote:
> >+static const struct net_dm_alert_ops *net_dm_alert_ops_arr[] = {
> >+	[NET_DM_ALERT_MODE_SUMMARY]	= &net_dm_alert_summary_ops,
> >+	[NET_DM_ALERT_MODE_PACKET]	= &net_dm_alert_packet_ops,
> >+};
> 
> Please split this patch into 2:
> 1) introducing the ops and modes (only summary)
> 2) introducing the packet mode

Ack

...

> >+static int net_dm_alert_mode_set(struct genl_info *info)
> >+{
> >+	struct netlink_ext_ack *extack = info->extack;
> >+	enum net_dm_alert_mode alert_mode;
> >+	int rc;
> >+
> >+	if (!info->attrs[NET_DM_ATTR_ALERT_MODE])
> >+		return 0;
> >+
> >+	rc = net_dm_alert_mode_get_from_info(info, &alert_mode);
> >+	if (rc) {
> >+		NL_SET_ERR_MSG_MOD(extack, "Invalid alert mode");
> >+		return -EINVAL;
> >+	}
> >+
> >+	net_dm_alert_mode = alert_mode;
> 
> 2 things:
> 1) Shouldn't you check if the tracing is on and return -EBUSY in case it is?

I'm doing it below in net_dm_cmd_config() :)
But I'm returning '-EOPNOTSUPP'. I guess '-EBUSY' is more appropriate.
Will change.

> 2) You setup the mode globally. I guess it is fine and it does not make
>    sense to do it otherwise, right? Like per-net or something.

Yes, it's global. I didn't change that aspect of drop monitor and I
don't really see a use case for that.

> 
> 
> >+
> >+	return 0;
> >+}
> >+
> > static int net_dm_cmd_config(struct sk_buff *skb,
> > 			struct genl_info *info)
> > {
> >-	NL_SET_ERR_MSG_MOD(info->extack, "Command not supported");
> >+	struct netlink_ext_ack *extack = info->extack;
> >+	int rc;
> > 
> >-	return -EOPNOTSUPP;
> >+	if (trace_state == TRACE_ON) {
> >+		NL_SET_ERR_MSG_MOD(extack, "Cannot configure drop monitor while tracing is on");
> >+		return -EOPNOTSUPP;
> >+	}
> >+
> >+	rc = net_dm_alert_mode_set(info);
> >+	if (rc)
> >+		return rc;
> >+
> >+	return 0;
> > }