[PATCH 0/4] sctp: do some clean up and fix comments

[PATCH 0/4] sctp: do some clean up and fix comments

[Wang Weidong]

I found: two if statements do the same work, we can merge to one;
kmem_cache_zalloc will reset the memory, no need do the work
which initialize with 0; some spelling errors, then fix them.

Wang Weidong (4):
  sctp: merge two if statements to one
  sctp: remove the repeat initialize with 0
  sctp: fix some comments in associola.c
  sctp: fix comment in chunk.c

 net/sctp/associola.c     |  4 ++--
 net/sctp/auth.c          | 12 ++++--------
 net/sctp/chunk.c         |  2 +-
 net/sctp/sm_make_chunk.c | 29 ++++++++---------------------
 4 files changed, 15 insertions(+), 32 deletions(-)

-- 
1.7.12

[PATCH 1/4] sctp: merge two if statements to one

[Wang Weidong]

Two if statements do the same work, maybe we can merge them to
one. There is just code simplification, no functional changes.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/auth.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/sctp/auth.c b/net/sctp/auth.c
index 8c4fa5d..19fb0ae 100644
--- a/net/sctp/auth.c
+++ b/net/sctp/auth.c
@@ -539,18 +539,14 @@ struct sctp_hmac *sctp_auth_asoc_get_hmac(const struct sctp_association *asoc)
 	for (i = 0; i < n_elt; i++) {
 		id = ntohs(hmacs->hmac_ids[i]);
 
-		/* Check the id is in the supported range */
-		if (id > SCTP_AUTH_HMAC_ID_MAX) {
-			id = 0;
-			continue;
-		}
-
-		/* See is we support the id.  Supported IDs have name and
+		/* Check the id is in the supported range. And
+		 * see is we support the id.  Supported IDs have name and
 		 * length fields set, so that we can allocated and use
 		 * them.  We can safely just check for name, for without the
 		 * name, we can't allocate the TFM.
 		 */
-		if (!sctp_hmac_list[id].hmac_name) {
+		if (id > SCTP_AUTH_HMAC_ID_MAX ||
+			!sctp_hmac_list[id].hmac_name) {
 			id = 0;
 			continue;
 		}
-- 
1.7.12

[PATCH 2/4] sctp: remove the repeat initialize with 0

[Wang Weidong]

kmem_cache_zalloc had set the allocated memory to zero. I think no need
to initialize with 0. And move the comments to the function begin.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/sm_make_chunk.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index d244a23..fe69032 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1297,6 +1297,13 @@ struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc)
 
 /* Turn an skb into a chunk.
  * FIXME: Eventually move the structure directly inside the skb->cb[].
+ *
+ * sctpimpguide-05.txt Section 2.8.2
+ * M1) Each time a new DATA chunk is transmitted
+ * set the 'TSN.Missing.Report' count for that TSN to 0. The
+ * 'TSN.Missing.Report' count will be used to determine missing chunks
+ * and when to fast retransmit.
+ *
  */
 struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
 			    const struct sctp_association *asoc,
@@ -1314,29 +1321,9 @@ struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
 	INIT_LIST_HEAD(&retval->list);
 	retval->skb		= skb;
 	retval->asoc		= (struct sctp_association *)asoc;
-	retval->has_tsn		= 0;
-	retval->has_ssn         = 0;
-	retval->rtt_in_progress	= 0;
-	retval->sent_at		= 0;
 	retval->singleton	= 1;
-	retval->end_of_packet	= 0;
-	retval->ecn_ce_done	= 0;
-	retval->pdiscard	= 0;
-
-	/* sctpimpguide-05.txt Section 2.8.2
-	 * M1) Each time a new DATA chunk is transmitted
-	 * set the 'TSN.Missing.Report' count for that TSN to 0. The
-	 * 'TSN.Missing.Report' count will be used to determine missing chunks
-	 * and when to fast retransmit.
-	 */
-	retval->tsn_missing_report = 0;
-	retval->tsn_gap_acked = 0;
-	retval->fast_retransmit = SCTP_CAN_FRTX;
 
-	/* If this is a fragmented message, track all fragments
-	 * of the message (for SEND_FAILED).
-	 */
-	retval->msg = NULL;
+	retval->fast_retransmit = SCTP_CAN_FRTX;
 
 	/* Polish the bead hole.  */
 	INIT_LIST_HEAD(&retval->transmitted_list);
-- 
1.7.12

[PATCH 3/4] sctp: fix some comments in associola.c

[Wang Weidong]

fix some spellings

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index cef5099..c9b91cb 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -602,7 +602,7 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc,
 
 		/* Start a T3 timer here in case it wasn't running so
 		 * that these migrated packets have a chance to get
-		 * retrnasmitted.
+		 * retransmitted.
 		 */
 		if (!timer_pending(&active->T3_rtx_timer))
 			if (!mod_timer(&active->T3_rtx_timer,
@@ -665,7 +665,7 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
 	/* Set the path max_retrans.  */
 	peer->pathmaxrxt = asoc->pathmaxrxt;
 
-	/* And the partial failure retrnas threshold */
+	/* And the partial failure retrans threshold */
 	peer->pf_retrans = asoc->pf_retrans;
 
 	/* Initialize the peer's SACK delay timeout based on the
-- 
1.7.12

[PATCH 4/4] sctp: fix comment in chunk.c

[Wang Weidong]

fix a spelling

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/chunk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index 7bd5ed4..f2044fc 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -201,7 +201,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
 
 	max = asoc->frag_point;
 	/* If the the peer requested that we authenticate DATA chunks
-	 * we need to accound for bundling of the AUTH chunks along with
+	 * we need to account for bundling of the AUTH chunks along with
 	 * DATA.
 	 */
 	if (sctp_auth_send_cid(SCTP_CID_DATA, asoc)) {
-- 
1.7.12

Re: [PATCH 1/4] sctp: merge two if statements to one

[Sergei Shtylyov]

Hello.

On 25-10-2013 5:50, Wang Weidong wrote:

> Two if statements do the same work, maybe we can merge them to
> one. There is just code simplification, no functional changes.

> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>   net/sctp/auth.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

    I understand what I noticed below is not your typos but maybe it's time to 
fix them?

> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 8c4fa5d..19fb0ae 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -539,18 +539,14 @@ struct sctp_hmac *sctp_auth_asoc_get_hmac(const struct sctp_association *asoc)
>   	for (i = 0; i < n_elt; i++) {
>   		id = ntohs(hmacs->hmac_ids[i]);
>
> -		/* Check the id is in the supported range */
> -		if (id > SCTP_AUTH_HMAC_ID_MAX) {
> -			id = 0;
> -			continue;
> -		}
> -
> -		/* See is we support the id.  Supported IDs have name and
> +		/* Check the id is in the supported range. And
> +		 * see is we support the id.  Supported IDs have name and

    s/is/if/.

>   		 * length fields set, so that we can allocated and use

    s/allocated/allocate/.

WBR, Sergei

Re: [PATCH 1/4] sctp: merge two if statements to one

[Vlad Yasevich]

On 10/24/2013 09:50 PM, Wang Weidong wrote:
> Two if statements do the same work, maybe we can merge them to
> one. There is just code simplification, no functional changes.
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>   net/sctp/auth.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index 8c4fa5d..19fb0ae 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -539,18 +539,14 @@ struct sctp_hmac *sctp_auth_asoc_get_hmac(const struct sctp_association *asoc)
>   	for (i = 0; i < n_elt; i++) {
>   		id = ntohs(hmacs->hmac_ids[i]);
>
> -		/* Check the id is in the supported range */
> -		if (id > SCTP_AUTH_HMAC_ID_MAX) {
> -			id = 0;
> -			continue;
> -		}
> -
> -		/* See is we support the id.  Supported IDs have name and
> +		/* Check the id is in the supported range. And
> +		 * see is we support the id.  Supported IDs have name and
>   		 * length fields set, so that we can allocated and use
>   		 * them.  We can safely just check for name, for without the
>   		 * name, we can't allocate the TFM.
>   		 	*/
> -		if (!sctp_hmac_list[id].hmac_name) {
> +		if (id > SCTP_AUTH_HMAC_ID_MAX ||
> +			!sctp_hmac_list[id].hmac_name) {

Can you please make the 2 parts of the 'if' statement above line up
with each other instead of the code below.  I makes it easy to see what
the whole 'if conditional' is.

Thanks
-vlad

>   			id = 0;
>   			continue;
>   		}
>

Re: [PATCH 2/4] sctp: remove the repeat initialize with 0

[Vlad Yasevich]

On 10/24/2013 09:50 PM, Wang Weidong wrote:
> kmem_cache_zalloc had set the allocated memory to zero. I think no need
> to initialize with 0. And move the comments to the function begin.
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>


Yes, thank you.

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/sm_make_chunk.c | 29 ++++++++---------------------
>   1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index d244a23..fe69032 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1297,6 +1297,13 @@ struct sctp_chunk *sctp_make_auth(const struct sctp_association *asoc)
>
>   /* Turn an skb into a chunk.
>    * FIXME: Eventually move the structure directly inside the skb->cb[].
> + *
> + * sctpimpguide-05.txt Section 2.8.2
> + * M1) Each time a new DATA chunk is transmitted
> + * set the 'TSN.Missing.Report' count for that TSN to 0. The
> + * 'TSN.Missing.Report' count will be used to determine missing chunks
> + * and when to fast retransmit.
> + *
>    */
>   struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
>   			    const struct sctp_association *asoc,
> @@ -1314,29 +1321,9 @@ struct sctp_chunk *sctp_chunkify(struct sk_buff *skb,
>   	INIT_LIST_HEAD(&retval->list);
>   	retval->skb		= skb;
>   	retval->asoc		= (struct sctp_association *)asoc;
> -	retval->has_tsn		= 0;
> -	retval->has_ssn         = 0;
> -	retval->rtt_in_progress	= 0;
> -	retval->sent_at		= 0;
>   	retval->singleton	= 1;
> -	retval->end_of_packet	= 0;
> -	retval->ecn_ce_done	= 0;
> -	retval->pdiscard	= 0;
> -
> -	/* sctpimpguide-05.txt Section 2.8.2
> -	 * M1) Each time a new DATA chunk is transmitted
> -	 * set the 'TSN.Missing.Report' count for that TSN to 0. The
> -	 * 'TSN.Missing.Report' count will be used to determine missing chunks
> -	 * and when to fast retransmit.
> -	 */
> -	retval->tsn_missing_report = 0;
> -	retval->tsn_gap_acked = 0;
> -	retval->fast_retransmit = SCTP_CAN_FRTX;
>
> -	/* If this is a fragmented message, track all fragments
> -	 * of the message (for SEND_FAILED).
> -	 */
> -	retval->msg = NULL;
> +	retval->fast_retransmit = SCTP_CAN_FRTX;
>
>   	/* Polish the bead hole.  */
>   	INIT_LIST_HEAD(&retval->transmitted_list);
>

Re: [PATCH 3/4] sctp: fix some comments in associola.c

[Vlad Yasevich]

On 10/24/2013 09:50 PM, Wang Weidong wrote:
> fix some spellings
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/associola.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index cef5099..c9b91cb 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -602,7 +602,7 @@ void sctp_assoc_rm_peer(struct sctp_association *asoc,
>
>   		/* Start a T3 timer here in case it wasn't running so
>   		 * that these migrated packets have a chance to get
> -		 * retrnasmitted.
> +		 * retransmitted.
>   		 */
>   		if (!timer_pending(&active->T3_rtx_timer))
>   			if (!mod_timer(&active->T3_rtx_timer,
> @@ -665,7 +665,7 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
>   	/* Set the path max_retrans.  */
>   	peer->pathmaxrxt = asoc->pathmaxrxt;
>
> -	/* And the partial failure retrnas threshold */
> +	/* And the partial failure retrans threshold */
>   	peer->pf_retrans = asoc->pf_retrans;
>
>   	/* Initialize the peer's SACK delay timeout based on the
>

Re: [PATCH 4/4] sctp: fix comment in chunk.c

[Vlad Yasevich]

On 10/24/2013 09:50 PM, Wang Weidong wrote:
> fix a spelling
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>

You could have combined these spelling fixes into 1 patch.

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

-vlad

> ---
>   net/sctp/chunk.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
> index 7bd5ed4..f2044fc 100644
> --- a/net/sctp/chunk.c
> +++ b/net/sctp/chunk.c
> @@ -201,7 +201,7 @@ struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *asoc,
>
>   	max = asoc->frag_point;
>   	/* If the the peer requested that we authenticate DATA chunks
> -	 * we need to accound for bundling of the AUTH chunks along with
> +	 * we need to account for bundling of the AUTH chunks along with
>   	 * DATA.
>   	 */
>   	if (sctp_auth_send_cid(SCTP_CID_DATA, asoc)) {
>

Re: [PATCH 1/4] sctp: merge two if statements to one

[wangweidong]

On 2013/10/25 21:04, Sergei Shtylyov wrote:
> Hello.
> 
> On 25-10-2013 5:50, Wang Weidong wrote:
> 
>> Two if statements do the same work, maybe we can merge them to
>> one. There is just code simplification, no functional changes.
> 
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/sctp/auth.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
>    I understand what I noticed below is not your typos but maybe it's time to fix them?

Yeah, I will fix them.
Thanks.

> 
>> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
>> index 8c4fa5d..19fb0ae 100644
>> --- a/net/sctp/auth.c
>> +++ b/net/sctp/auth.c
>> @@ -539,18 +539,14 @@ struct sctp_hmac *sctp_auth_asoc_get_hmac(const struct sctp_association *asoc)
>>       for (i = 0; i < n_elt; i++) {
>>           id = ntohs(hmacs->hmac_ids[i]);
>>
>> -        /* Check the id is in the supported range */
>> -        if (id > SCTP_AUTH_HMAC_ID_MAX) {
>> -            id = 0;
>> -            continue;
>> -        }
>> -
>> -        /* See is we support the id.  Supported IDs have name and
>> +        /* Check the id is in the supported range. And
>> +         * see is we support the id.  Supported IDs have name and
> 
>    s/is/if/.
> 
>>            * length fields set, so that we can allocated and use
> 
>    s/allocated/allocate/.
> 
> WBR, Sergei
> 
> 
> 

Re: [PATCH 1/4] sctp: merge two if statements to one

[wangweidong]

On 2013/10/26 7:40, Vlad Yasevich wrote:
> On 10/24/2013 09:50 PM, Wang Weidong wrote:
>> Two if statements do the same work, maybe we can merge them to
>> one. There is just code simplification, no functional changes.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/sctp/auth.c | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
>> index 8c4fa5d..19fb0ae 100644
>> --- a/net/sctp/auth.c
>> +++ b/net/sctp/auth.c
>> @@ -539,18 +539,14 @@ struct sctp_hmac *sctp_auth_asoc_get_hmac(const struct sctp_association *asoc)
>>       for (i = 0; i < n_elt; i++) {
>>           id = ntohs(hmacs->hmac_ids[i]);
>>
>> -        /* Check the id is in the supported range */
>> -        if (id > SCTP_AUTH_HMAC_ID_MAX) {
>> -            id = 0;
>> -            continue;
>> -        }
>> -
>> -        /* See is we support the id.  Supported IDs have name and
>> +        /* Check the id is in the supported range. And
>> +         * see is we support the id.  Supported IDs have name and
>>            * length fields set, so that we can allocated and use
>>            * them.  We can safely just check for name, for without the
>>            * name, we can't allocate the TFM.
>>                */
>> -        if (!sctp_hmac_list[id].hmac_name) {
>> +        if (id > SCTP_AUTH_HMAC_ID_MAX ||
>> +            !sctp_hmac_list[id].hmac_name) {
> 
> Can you please make the 2 parts of the 'if' statement above line up
> with each other instead of the code below.  I makes it easy to see what
> the whole 'if conditional' is.
> 
> Thanks
> -vlad
> 

Ok, I will resend a new version.
Thanks. 

>>               id = 0;
>>               continue;
>>           }
>>
> 
> 
>