| Message ID | 20100906122344.GA2764@bicker |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Dan Carpenter wrote, at 09/06/2010 08:26 PM:
> + &new_addr->transports != &new_asoc->peer.transport_addr_list) {
why did you add this check?
On Tue, Sep 07, 2010 at 04:46:58PM +0800, Shan Wei wrote: > Dan Carpenter wrote, at 09/06/2010 08:26 PM: > > + &new_addr->transports != &new_asoc->peer.transport_addr_list) { > > why did you add this check? > That's the check which tells us if we broke out of the loop or if we came to the end of the list. As I explained before, the only way that the check matters is if the list is empty. With the current code I do not think we ever call this function with an empty list, so that check is not needed. But the code could change I suppose and it doesn't hurt to be cautious. On the other hand, I'm fine with removing the check as well. regards, dan carpenter > > -- > Best Regards > ----- > Shan Wei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/06/2010 08:26 AM, Dan Carpenter wrote: > "new_addr" is the list cursor here and it's always non-NULL. > > We're trying to test if we exited because the loop ended or we hit the > break statement. Really testing !found is enough so long as > "new_asoc->peer.transport_addr_list" is not empty and I believe it never > is empty at this point. So this is never really a bug with the current > code. > > Signed-off-by: Dan Carpenter <error27@gmail.com> > --- > Compile tested only. > > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c > index 24b2cd5..cb76d2e 100644 > --- a/net/sctp/sm_statefuns.c > +++ b/net/sctp/sm_statefuns.c > @@ -1254,7 +1254,6 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, > /* Search through all current addresses and make sure > * we aren't adding any new ones. > */ > - new_addr = NULL; > found = 0; > > list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list, > @@ -1273,7 +1272,8 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, > } > > /* If a new address was added, ABORT the sender. */ > - if (!found && new_addr) { > + if (!found && > + &new_addr->transports != &new_asoc->peer.transport_addr_list) { I am really not fond of this code. I think it would be better to move this abort code into the loop and break out once things are aborted. This way we can get rid of the whole found check after the outer loop happens and it cleans things up nicely. -vlad > sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands); > } > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Vlad Yasevich <vladislav.yasevich@hp.com> Date: Wed, 08 Sep 2010 16:26:45 -0400 > I am really not fond of this code. I think it would be better to move > this abort code into the loop and break out once things are aborted. > > This way we can get rid of the whole found check after the outer loop happens > and it cleans things up nicely. Vlad see my followup. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c index 24b2cd5..cb76d2e 100644 --- a/net/sctp/sm_statefuns.c +++ b/net/sctp/sm_statefuns.c @@ -1254,7 +1254,6 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, /* Search through all current addresses and make sure * we aren't adding any new ones. */ - new_addr = NULL; found = 0; list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list, @@ -1273,7 +1272,8 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, } /* If a new address was added, ABORT the sender. */ - if (!found && new_addr) { + if (!found && + &new_addr->transports != &new_asoc->peer.transport_addr_list) { sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands); }
"new_addr" is the list cursor here and it's always non-NULL. We're trying to test if we exited because the loop ended or we hit the break statement. Really testing !found is enough so long as "new_asoc->peer.transport_addr_list" is not empty and I believe it never is empty at this point. So this is never really a bug with the current code. Signed-off-by: Dan Carpenter <error27@gmail.com> --- Compile tested only. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html