Re: [PATCH net] sctp: remove temporary variable confirm from sctp_packet_transmit

[David Miller]

From: Xin Long 
Date: Sat, 18 Mar 2017 19:12:22 +0800

> Commit c86a773c7802 ("sctp: add dst_pending_confirm flag") introduced
> a temporary variable "confirm" in sctp_packet_transmit.
> 
> But it broke the rule that longer lines should be above shorter ones.
> Besides, this variable is not necessary, so this patch is to just
> remove it and use tp->dst_pending_confirm directly.
> 
> Fixes: c86a773c7802 ("sctp: add dst_pending_confirm flag")
> Signed-off-by: Xin Long 

Applied.

Re: [PATCH net] sctp: remove temporary variable confirm from sctp_packet_transmit

[Neil Horman]

On Sat, Mar 18, 2017 at 07:12:22PM +0800, Xin Long wrote:
> Commit c86a773c7802 ("sctp: add dst_pending_confirm flag") introduced
> a temporary variable "confirm" in sctp_packet_transmit.
> 
> But it broke the rule that longer lines should be above shorter ones.
> Besides, this variable is not necessary, so this patch is to just
> remove it and use tp->dst_pending_confirm directly.
> 
> Fixes: c86a773c7802 ("sctp: add dst_pending_confirm flag")
> Signed-off-by: Xin Long 
> ---
>  net/sctp/output.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 71ce6b9..1224421 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -546,7 +546,6 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
> gfp_t gfp)
>   struct sctp_association *asoc = tp->asoc;
>   struct sctp_chunk *chunk, *tmp;
>   int pkt_count, gso = 0;
> - int confirm;
>   struct dst_entry *dst;
>   struct sk_buff *head;
>   struct sctphdr *sh;
> @@ -625,13 +624,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
> gfp_t gfp)
>   asoc->peer.last_sent_to = tp;
>   }
>   head->ignore_df = packet->ipfragok;
> - confirm = tp->dst_pending_confirm;
> - if (confirm)
> + if (tp->dst_pending_confirm)
>   skb_set_dst_pending_confirm(head, 1);
>   /* neighbour should be confirmed on successful transmission or
>* positive error
>*/
> - if (tp->af_specific->sctp_xmit(head, tp) >= 0 && confirm)
> + if (tp->af_specific->sctp_xmit(head, tp) >= 0 &&
> + tp->dst_pending_confirm)
>   tp->dst_pending_confirm = 0;
>  
>  out:
> -- 
> 2.1.0
> 
> 
Acked-by: Neil Horman 

Re: [PATCH net] sctp: remove temporary variable confirm from sctp_packet_transmit

[Xin Long]

On Sat, Mar 18, 2017 at 7:48 PM, Julian Anastasov  wrote:
>
> Hello,
>
> On Sat, 18 Mar 2017, Xin Long wrote:
>
>> Commit c86a773c7802 ("sctp: add dst_pending_confirm flag") introduced
>> a temporary variable "confirm" in sctp_packet_transmit.
>>
>> But it broke the rule that longer lines should be above shorter ones.
>> Besides, this variable is not necessary, so this patch is to just
>> remove it and use tp->dst_pending_confirm directly.
>>
>> Fixes: c86a773c7802 ("sctp: add dst_pending_confirm flag")
>> Signed-off-by: Xin Long 
>> ---
>>  net/sctp/output.c | 7 +++
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sctp/output.c b/net/sctp/output.c
>> index 71ce6b9..1224421 100644
>> --- a/net/sctp/output.c
>> +++ b/net/sctp/output.c
>> @@ -546,7 +546,6 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
>> gfp_t gfp)
>>   struct sctp_association *asoc = tp->asoc;
>>   struct sctp_chunk *chunk, *tmp;
>>   int pkt_count, gso = 0;
>> - int confirm;
>>   struct dst_entry *dst;
>>   struct sk_buff *head;
>>   struct sctphdr *sh;
>> @@ -625,13 +624,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
>> gfp_t gfp)
>>   asoc->peer.last_sent_to = tp;
>>   }
>>   head->ignore_df = packet->ipfragok;
>> - confirm = tp->dst_pending_confirm;
>> - if (confirm)
>> + if (tp->dst_pending_confirm)
>>   skb_set_dst_pending_confirm(head, 1);
>>   /* neighbour should be confirmed on successful transmission or
>>* positive error
>>*/
>> - if (tp->af_specific->sctp_xmit(head, tp) >= 0 && confirm)
>> + if (tp->af_specific->sctp_xmit(head, tp) >= 0 &&
>> + tp->dst_pending_confirm)
>>   tp->dst_pending_confirm = 0;
>>
>>  out:
>> --
>
> I played safe here, I was not sure if currently
> or some day in the future the SCTP stack can allow another
> thread to set tp->dst_pending_confirm concurrently with the
> sending. My idea was only when skb was used to confirm
> neighbour only then to clear the indication. I guess, your
> patch is ok because we should be locking the socket
> everywhere.
Yeps, It's safe, as all the codes for dst_pending_confirm are
under the sock lock protection.

>
> Regards
>
> --
> Julian Anastasov 

Re: [PATCH net] sctp: remove temporary variable confirm from sctp_packet_transmit

[Julian Anastasov]

Hello,

On Sat, 18 Mar 2017, Xin Long wrote:

> Commit c86a773c7802 ("sctp: add dst_pending_confirm flag") introduced
> a temporary variable "confirm" in sctp_packet_transmit.
> 
> But it broke the rule that longer lines should be above shorter ones.
> Besides, this variable is not necessary, so this patch is to just
> remove it and use tp->dst_pending_confirm directly.
> 
> Fixes: c86a773c7802 ("sctp: add dst_pending_confirm flag")
> Signed-off-by: Xin Long 
> ---
>  net/sctp/output.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 71ce6b9..1224421 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -546,7 +546,6 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
> gfp_t gfp)
>   struct sctp_association *asoc = tp->asoc;
>   struct sctp_chunk *chunk, *tmp;
>   int pkt_count, gso = 0;
> - int confirm;
>   struct dst_entry *dst;
>   struct sk_buff *head;
>   struct sctphdr *sh;
> @@ -625,13 +624,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
> gfp_t gfp)
>   asoc->peer.last_sent_to = tp;
>   }
>   head->ignore_df = packet->ipfragok;
> - confirm = tp->dst_pending_confirm;
> - if (confirm)
> + if (tp->dst_pending_confirm)
>   skb_set_dst_pending_confirm(head, 1);
>   /* neighbour should be confirmed on successful transmission or
>* positive error
>*/
> - if (tp->af_specific->sctp_xmit(head, tp) >= 0 && confirm)
> + if (tp->af_specific->sctp_xmit(head, tp) >= 0 &&
> + tp->dst_pending_confirm)
>   tp->dst_pending_confirm = 0;
>  
>  out:
> -- 

I played safe here, I was not sure if currently
or some day in the future the SCTP stack can allow another
thread to set tp->dst_pending_confirm concurrently with the
sending. My idea was only when skb was used to confirm
neighbour only then to clear the indication. I guess, your
patch is ok because we should be locking the socket
everywhere.

Regards

--
Julian Anastasov 

[PATCH net] sctp: remove temporary variable confirm from sctp_packet_transmit

[Xin Long]

Commit c86a773c7802 ("sctp: add dst_pending_confirm flag") introduced
a temporary variable "confirm" in sctp_packet_transmit.

But it broke the rule that longer lines should be above shorter ones.
Besides, this variable is not necessary, so this patch is to just
remove it and use tp->dst_pending_confirm directly.

Fixes: c86a773c7802 ("sctp: add dst_pending_confirm flag")
Signed-off-by: Xin Long 
---
 net/sctp/output.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/sctp/output.c b/net/sctp/output.c
index 71ce6b9..1224421 100644
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -546,7 +546,6 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t 
gfp)
struct sctp_association *asoc = tp->asoc;
struct sctp_chunk *chunk, *tmp;
int pkt_count, gso = 0;
-   int confirm;
struct dst_entry *dst;
struct sk_buff *head;
struct sctphdr *sh;
@@ -625,13 +624,13 @@ int sctp_packet_transmit(struct sctp_packet *packet, 
gfp_t gfp)
asoc->peer.last_sent_to = tp;
}
head->ignore_df = packet->ipfragok;
-   confirm = tp->dst_pending_confirm;
-   if (confirm)
+   if (tp->dst_pending_confirm)
skb_set_dst_pending_confirm(head, 1);
/* neighbour should be confirmed on successful transmission or
 * positive error
 */
-   if (tp->af_specific->sctp_xmit(head, tp) >= 0 && confirm)
+   if (tp->af_specific->sctp_xmit(head, tp) >= 0 &&
+   tp->dst_pending_confirm)
tp->dst_pending_confirm = 0;
 
 out:
-- 
2.1.0