: Resolve dependencies of features on choice of CCID

[DCCP]: Resolve dependencies of features on choice of CCID

This provides a missing link in the code chain, as several features implicitly depend and/or rely on the choice of CCID. Most notably, this is the Send Ack Vector feature, but the patch also takes care of Ack Ratio and Send Loss Event Rate.

For Send Ack Vector, the situation is as follows:
* since CCID2 mandates the use of Ack Vectors, there is no point in allowing endpoints which use CCID2 to disable Send Ack Vector features on the corresponding half-connection;

* a peer with a TX CCID of CCID2 will always expect Ack Vectors, and a peer with a RX CCID of CCID2 must always send Ack Vectors (RFC 4341, sec. 4);

* for all other CCIDs, the use of (Send) Ack Vector is optional and thus negotiable. However, this implies that the code negotiating the use of Ack Vectors also supports it (i.e. is able to supply and to either parse or ignore received Ack Vectors). Since this is not the case (CCID3 has no Ack Vector support), the use of Ack Vectors is here disabled, with a corresponding comment in the source code.

An analogous consideration arises for the Send Loss Event Rate feature, since the CCID3 implementation does not support the loss interval options of RFC 4342. To make such use explicit, corresponding feature-negotiation options are inserted which signal the use of the loss event rate option, as it is used by the CCID3 code.

Lastly, the values of the Ack Ratio feature are matched to the choice of CCID.

The patch implements this as a function which is called after the user has made all other registrations for changing default values of features.

---
 net/dccp/dccp.h   |    1 
 net/dccp/feat.c   |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 net/dccp/feat.h   |   14 ++++++
 net/dccp/output.c |    4 +
 net/dccp/proto.c  |    3 +
 5 files changed, 137 insertions(+)

@@ -74,6 +74,20 @@ static inline u8 dccp_feat_genopt(struct
 	return entry->is_local? DCCPO_CHANGE_L : DCCPO_CHANGE_R;
 }
 
+/**
+ * ccid_dependency  -  Record and track changes resulting from changing the CCID
+ * @feat_num: one of %dccp_feature_numbers
+ * @is_local: local (1) or remote (0) feature
+ * @is_mandatory: whether presence of @val is critical or not
+ * @val: corresponding default value for @feat_num (u8 is sufficient here)
+ */
+typedef struct {
+	u8		feat_num;
+	bool		is_local:1,
+			is_mandatory:1;
+	u8 const	*val;
+} ccid_dependency;
+
 #ifdef CONFIG_IP_DCCP_DEBUG
 extern const char *dccp_feat_typename(const u8 type);
 extern const char *dccp_feat_name(const u8 feat);

@@ -422,6 +422,7 @@ static inline int dccp_ack_pending(const
 	       inet_csk_ack_scheduled(sk);
 }
 
+extern int  dccp_feat_finalise_settings(struct dccp_sock *dp);
 extern void dccp_feat_list_purge(struct list_head *fn_list);
 
 extern int dccp_insert_options(struct sock *sk, struct sk_buff *skb);

@@ -445,6 +445,10 @@ int dccp_connect(struct sock *sk)
 	struct sk_buff *skb;
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
+	/* do not connect if feature negotiation setup fails */
+	if (dccp_feat_finalise_settings(dccp_sk(sk)))
+		return -EPROTO;
+
 	dccp_connect_init(sk);
 
 	skb = alloc_skb(sk->sk_prot->max_header, sk->sk_allocation);

@@ -282,6 +282,9 @@ static inline int dccp_listen_start(stru
 	struct dccp_sock *dp = dccp_sk(sk);
 
 	dp->dccps_role = DCCP_ROLE_LISTEN;
+	/* do not start to listen if feature negotiation setup fails */
+	if (dccp_feat_finalise_settings(dp))
+		return -EPROTO;
 	return inet_csk_listen_start(sk, backlog);
 }
 

I agree totally with the intent of this patch as long thought it was silly the ack vectors situation.

I don't quite understand why you're defining on and off when there is built in stuff for the kernel for true/false. You then pass the address of the on/off - not the value. Maybe you might like to explain the rationale behind this as I'm probably missing something.

(Sending acked bys these days as much less time so giving quick reviews. I'm working through all the new group of patches from oldest first. I've lost track of any old ones I haven't reviewed so if you still want me to do these can you send me a list of patch sets or git commit IDs)

|  I don't quite understand why you're defining on and off when there is
|  built in stuff for the kernel for true/false. You then pass the
|  address of the on/off - not the value. Maybe you might like to explain
|  the rationale behind this as I'm probably missing something.

No you are not - thanks so much for pointing this out! These constants have been a constant eyesore and I had wanted to get rid of them, but lacked an idea.

The pointer was used to have a sentinel in the table, since the caller can not know how large the table is. But a look at RFC 4340, 6.4 showed that `0' is never a valid feature number, so that is used as an end-of-table condition now.

Thanks a lot.