Em Tue, Oct 02, 2007 at 01:06:55PM +0100, Gerrit Renker escreveu:
> Reworked patch thanks to suggestions by Ian. Changes:
> * removed on/off constants
> * used reserved feature number instead of reserved pointer value (NULL)
> to mark the end of table; marked this in the commit message
> * all boolean types are now initialised with `true'/`false', while the
> actual feature-negotiation value (not a boolean) is used for initialisation
> * compile-tested and checked against old patch
>
> ------------------------------> Patch v2 <-----------------------------------------
> [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.
>
> The table is variable-length, the reserved (and hence for feature-negotiation invalid, confirmed
> by considering section 19.4 of RFC 4340) feature number of 0 is used to mark the end of the table.
>
> Signed-off-by: Gerrit Renker <gerrit@xxxxxxxxxxxxxx>
> ---
> 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(+)
>
> --- a/net/dccp/feat.h
> +++ b/net/dccp/feat.h
> @@ -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 const struct {
What is the effect of const being used in a struct typedef?
> + u8 feat_num;
> + bool is_local:1,
> + is_mandatory:1;
> + u8 val;
> +} ccid_dependency;
And why a typedef? Reasons why typedefs are frown upon have been many
times said.
> +
> #ifdef CONFIG_IP_DCCP_DEBUG
> extern const char *dccp_feat_typename(const u8 type);
> extern const char *dccp_feat_name(const u8 feat);
> --- a/net/dccp/dccp.h
> +++ b/net/dccp/dccp.h
> @@ -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);
> --- a/net/dccp/feat.c
> +++ b/net/dccp/feat.c
> @@ -432,6 +432,121 @@ int dccp_feat_change(struct dccp_minisoc
>
> EXPORT_SYMBOL_GPL(dccp_feat_change);
>
> +/*
> + * Tracking features whose value depend on the choice of CCID
> + *
> + * This is designed with an extension in mind so that a list walk could be done
> + * before activating any features. However, the existing framework was found to
> + * work satisfactorily up until now, the automatic verification is left open.
> + */
> +static ccid_dependency *dccp_feat_ccid_dependency(u8 ccid, bool is_local)
> +{
> + static ccid_dependency ccid2_dependencies[2][2] = {
> + /*
> + * CCID2 mandates Ack Vectors (RFC 4341, 4.): since CCID is a TX
> + * feature and Send Ack Vector is an RX feature, `is_local'
> + * needs to be reversed.
> + */
> + { /* !is_local: dependencies of the remote (RX) CCID */
> + { DCCPF_SEND_ACK_VECTOR, true, true, 1 },
> + { 0, 0, 0, 0 }
designated initializers, please, that way we can avoid the zeroes and
see the name of the fields, which helps understanding without having to
go back to the struct definition.
> + },
> + { /* is_local: choices for the local (TX) CCID */
> + { DCCPF_SEND_ACK_VECTOR, false, true, 1 },
> + { 0, 0, 0, 0 }
> + }
> + };
> + static ccid_dependency ccid3_dependencies[2][5] = {
> + /*
> + * CCID3 at the RX side: we locally disable Ack Vectors; enable
> + * Send Loss Event Rate (for reasons below); and we request NDP
> + * options (which are used/needed as per RFC 4342, 6.1.1).
> + */
> + {
> + { DCCPF_SEND_ACK_VECTOR, true, false, 0 },
> + { DCCPF_SEND_LEV_RATE, true, true, 1 },
> + { DCCPF_SEND_NDP_COUNT, false, true, 1 },
> + { 0, 0, 0, 0 },
> + }, {
> + /*
> + * CCID3 at the TX side: we request that the HC-receiver will
> + * not send Ack Vectors (they will be ignored, so `Mandatory' is
> + * not set); we set Send Loss Event Rate to 1 (Mandatory since
> + * the implementation does not support the Loss Intervals option
> + * of RFC 4342, 8.6). The last two options are for information
> + * only (not mandatory): this CCID does not support Ack Ratio,
> + * but NDP Count options will be sent.
> + */
> + { DCCPF_SEND_ACK_VECTOR, false, false, 0 },
> + { DCCPF_SEND_LEV_RATE, false, true, 1 },
> + { DCCPF_ACK_RATIO, true, false, 0 },
> + { DCCPF_SEND_NDP_COUNT, true, false, 1 },
> + { 0, 0, 0, 0 }
> + }
> + };
> + switch (ccid) {
> + case DCCPC_CCID2: return ccid2_dependencies[is_local];
> + case DCCPC_CCID3: return ccid3_dependencies[is_local];
> + default: return NULL; /* other CCIDs: no specifics yet */
> + }
> +}
> +
> +/**
> + * dccp_feat_propagate_ccid - Resolve dependencies of features on choice of CCID
> + * @fn: feature-negotiation list to update
> + * @id: CCID number to track
> + * @is_local: whether TX CCID (1) or RX CCID (0) is meant
> + * This function needs to be called after registering all other features.
> + */
> +static int dccp_feat_propagate_ccid(struct list_head *fn, u8 id, bool is_local)
> +{
> + ccid_dependency *table = dccp_feat_ccid_dependency(id, is_local);
> + u8 i, rc = (table == NULL);
> +
> + for (i = 0; rc == 0 && table[i].feat_num != DCCPF_RESERVED; i++)
> + if (dccp_feat_type(table[i].feat_num) == FEAT_SP)
> + rc = dccp_feat_register_sp(fn, table[i].feat_num,
> + table[i].is_local,
> + table[i].is_mandatory,
> + &table[i].val, 1);
> + else
> + rc = dccp_feat_register_nn(fn, table[i].feat_num,
> + table[i].is_mandatory,
> + table[i].val);
> + return rc;
> +}
> +
> +/**
> + * dccp_feat_finalise_settings - Finalise settings before starting negotiation
> + * @dp: client or listening socket (settings will be inherited)
> + * This is called after all registrations (socket initialisation, sysctls, and
> + * sockopt calls), and before sending the first packet containing Change options
> + * (ie. client-Request or server-Response), to ensure internal consistency.
> + */
> +int dccp_feat_finalise_settings(struct dccp_sock *dp)
> +{
> + struct list_head *fn = &dp->dccps_featneg;
> + struct dccp_feat_entry *entry;
> + int i = 2, ccids[2] = { -1, -1 };
> +
> + /*
> + * Propagating CCIDs:
> + * 1) not useful to propagate CCID settings if this host advertises more
> + * than one CCID: the choice of CCID may still change - if this is
> + * the client, or if this is the server and the client sends
> + * singleton CCID values.
> + * 2) since is that propagate_ccid changes the list, we defer changing
> + * the sorted list until after the traversal.
> + */
> + list_for_each_entry(entry, fn, node)
> + if (entry->feat_num == DCCPF_CCID && entry->val.sp.len == 1)
> + ccids[entry->is_local] = entry->val.sp.vec[0];
> + while (i--)
> + if (ccids[i] > 0 && dccp_feat_propagate_ccid(fn, ccids[i], i))
> + return -1;
> + return 0;
> +}
> +
> static int dccp_feat_update_ccid(struct sock *sk, u8 type, u8 new_ccid_nr)
> {
> struct dccp_sock *dp = dccp_sk(sk);
> --- a/net/dccp/output.c
> +++ b/net/dccp/output.c
> @@ -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);
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -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);
> }
>
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
