Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-25 Thread Jarno Rajahalme

On Sep 25, 2014, at 7:52 AM, Daniele Venturino daniele.ventur...@m3s.it wrote:

 After some testing, here’s my ack.
 
 Acked-by: Daniele Venturino daniele.ventur...@m3s.it
 

Pushed to master, thanks!

  Jarno

 
 diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
 index d3e527a..d3973e5 100644
 --- a/ofproto/ofproto-dpif.c
 +++ b/ofproto/ofproto-dpif.c
 @@ -2386,6 +2386,8 @@ set_rstp_port(struct ofport *ofport_,
  rstp_port_set(rp, s-port_num, s-priority, s-path_cost,
s-admin_edge_port, s-auto_edge, s-mcheck, ofport);
  update_rstp_port_state(ofport);
 +/* Synchronize operational status. */
 +rstp_port_set_mac_operational(rp, ofport-may_enable);
  }
  
  static void
 
 This helps, physical interfaces are no longer blocked in role disabled if 
 RSTP is enabled after they are added to a bridge.
 I’ll test this more thoroughly soon.
 
 
 I’ll merge this when I get your “Acked-by:”,
 
   Jarno
 

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev


Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-19 Thread Daniele Venturino

Il giorno 11/set/2014, alle ore 19:09, Jarno Rajahalme jrajaha...@nicira.com 
ha scritto:

 
 On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it 
 wrote:
 
 
 Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino 
 daniele.ventur...@m3s.it ha scritto:
 
 
 Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme 
 jrajaha...@nicira.com ha scritto:
 
 
 On Sep 9, 2014, at 3:53 AM, Daniele Venturino 
 venturino.dani...@gmail.com wrote:
 
 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I 
 plan to squash trivial CodingStyle fixes in before pushing the series to 
 master. Also, I’ll add a News item stating that experimental RSTP is 
 added, and more compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.
 
 Thanks!
 
 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working with 
 an IXIA validation software for a couple of weeks now, and I already have 
 some patches.
 I'm still solving some problems and I expect to have some more fixes. 
 This should take a couple of weeks to obtain a compliant version, so 
 maybe it would be better to wait for a compliant version of the protocol 
 before merging it to the master. Anyway, if you still want to merge it 
 and mark it as experimental is still fine by me, since I'll be able to 
 rebase my patches on your version.
 
 
 Nice to hear about the IXIA validation work. I’ll merge the series to 
 master soon, so that we have less different versions around.
 
 Thanks to you for your reviews! I’ll let you have my other patches soon.
 
 
 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino 
 venturino.dani...@gmail.com wrote:
 I looked and applied the patches. They’re good to me, I just have some 
 notes on patch 13/18 and 16/18.
 
 
 (snip)
 
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, 
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  
 RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the 
 bridge.
  
 I will look into this.
 
 Later.
 
 (snip)
 
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add 
 here this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by adding 
 ovs-vsctl del-br and del-port commands to the new test cases, and they 
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. 
 I see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct 
 xport *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.
 
 Good :-)
 
  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const 
 void *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = 

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-19 Thread Jarno Rajahalme

On Sep 19, 2014, at 8:26 AM, Daniele Venturino daniele.ventur...@m3s.it wrote:

 
 Il giorno 11/set/2014, alle ore 19:09, Jarno Rajahalme 
 jrajaha...@nicira.com ha scritto:
 
 
 On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it 
 wrote:
 
 
 Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino 
 daniele.ventur...@m3s.it ha scritto:
 
 
 Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme 
 jrajaha...@nicira.com ha scritto:
 
 
 On Sep 9, 2014, at 3:53 AM, Daniele Venturino 
 venturino.dani...@gmail.com wrote:
 
 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I 
 plan to squash trivial CodingStyle fixes in before pushing the series to 
 master. Also, I’ll add a News item stating that experimental RSTP is 
 added, and more compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.
 
 Thanks!
 
 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working 
 with an IXIA validation software for a couple of weeks now, and I 
 already have some patches.
 I'm still solving some problems and I expect to have some more fixes. 
 This should take a couple of weeks to obtain a compliant version, so 
 maybe it would be better to wait for a compliant version of the protocol 
 before merging it to the master. Anyway, if you still want to merge it 
 and mark it as experimental is still fine by me, since I'll be able to 
 rebase my patches on your version.
 
 
 Nice to hear about the IXIA validation work. I’ll merge the series to 
 master soon, so that we have less different versions around.
 
 Thanks to you for your reviews! I’ll let you have my other patches soon.
 
 
 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino 
 venturino.dani...@gmail.com wrote:
 I looked and applied the patches. They’re good to me, I just have some 
 notes on patch 13/18 and 16/18.
 
 
 (snip)
 
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, 
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  
 RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). 
 We could define a funcion like rstp_initialize_port_defaults__() for the 
 bridge.
  
 I will look into this.
 
 Later.
 
 (snip)
 
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int 
 rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum 
 ofputil_port_state state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add 
 here this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by 
 adding ovs-vsctl del-br and del-port commands to the new test cases, and 
 they succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. 
 I see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct 
 xport *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.
 
 Good :-)
 
  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const 
 void *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-11 Thread Daniele Venturino

Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino 
daniele.ventur...@m3s.it ha scritto:

 
 Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme 
 jrajaha...@nicira.com ha scritto:
 
 
 On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 
 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I 
 plan to squash trivial CodingStyle fixes in before pushing the series to 
 master. Also, I’ll add a News item stating that experimental RSTP is added, 
 and more compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.
 
 Thanks!
 
 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working with 
 an IXIA validation software for a couple of weeks now, and I already have 
 some patches.
 I'm still solving some problems and I expect to have some more fixes. This 
 should take a couple of weeks to obtain a compliant version, so maybe it 
 would be better to wait for a compliant version of the protocol before 
 merging it to the master. Anyway, if you still want to merge it and mark it 
 as experimental is still fine by me, since I'll be able to rebase my 
 patches on your version.
 
 
 Nice to hear about the IXIA validation work. I’ll merge the series to master 
 soon, so that we have less different versions around.
 
 Thanks to you for your reviews! I’ll let you have my other patches soon.
 
 
 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 I looked and applied the patches. They’re good to me, I just have some 
 notes on patch 13/18 and 16/18.
 
 
 (snip)
 
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, 
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  
 RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the 
 bridge.
  
 I will look into this.
 
 Later.
 
 (snip)
 
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add here 
 this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by adding 
 ovs-vsctl del-br and del-port commands to the new test cases, and they 
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. I 
 see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct xport 
 *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.
 
 Good :-)
 
  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void 
 *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = rstp_port_ref(rstp_port);
 +}
 @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
  if (ofport-may_enable != enable) {
  struct ofproto_dpif *ofproto = 
 

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-11 Thread Jarno Rajahalme

On Sep 11, 2014, at 5:49 AM, Daniele Venturino daniele.ventur...@m3s.it wrote:

 
 Il giorno 09/set/2014, alle ore 22:04, Daniele Venturino 
 daniele.ventur...@m3s.it ha scritto:
 
 
 Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme 
 jrajaha...@nicira.com ha scritto:
 
 
 On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 
 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I 
 plan to squash trivial CodingStyle fixes in before pushing the series to 
 master. Also, I’ll add a News item stating that experimental RSTP is 
 added, and more compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.
 
 Thanks!
 
 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working with 
 an IXIA validation software for a couple of weeks now, and I already have 
 some patches.
 I'm still solving some problems and I expect to have some more fixes. This 
 should take a couple of weeks to obtain a compliant version, so maybe it 
 would be better to wait for a compliant version of the protocol before 
 merging it to the master. Anyway, if you still want to merge it and mark 
 it as experimental is still fine by me, since I'll be able to rebase my 
 patches on your version.
 
 
 Nice to hear about the IXIA validation work. I’ll merge the series to 
 master soon, so that we have less different versions around.
 
 Thanks to you for your reviews! I’ll let you have my other patches soon.
 
 
 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino 
 venturino.dani...@gmail.com wrote:
 I looked and applied the patches. They’re good to me, I just have some 
 notes on patch 13/18 and 16/18.
 
 
 (snip)
 
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, 
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  
 RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the 
 bridge.
  
 I will look into this.
 
 Later.
 
 (snip)
 
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add here 
 this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by adding 
 ovs-vsctl del-br and del-port commands to the new test cases, and they 
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. I 
 see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct 
 xport *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.
 
 Good :-)
 
  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const 
 void *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = rstp_port_ref(rstp_port);
 +}
 @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
  if 

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-09 Thread Daniele Venturino


 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I
 plan to squash trivial CodingStyle fixes in before pushing the series to
 master. Also, I’ll add a News item stating that experimental RSTP is added,
 and more compliance and interoperability testing is needed.
 Responses below,
   Jarno


Ok. I sent my acks.

I did some minor changes from June to August, that you can see here:
https://github.com/M3S/ovs-rstp/compare/b946221...c910081

About the compliance and interoperability testing, I've been working with
an IXIA validation software for a couple of weeks now, and I already have
some patches.
I'm still solving some problems and I expect to have some more fixes. This
should take a couple of weeks to obtain a compliant version, so maybe it
would be better to wait for a compliant version of the protocol before
merging it to the master. Anyway, if you still want to merge it and mark it
as experimental is still fine by me, since I'll be able to rebase my
patches on your version.


On Sep 3, 2014, at 7:44 AM, Daniele Venturino venturino.dani...@gmail.com
 wrote:
 I looked and applied the patches. They’re good to me, I just have some
 notes on patch 13/18 and 16/18.

  memcpy(p-received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
  rstp-changes = true;
 -move_rstp(rstp);
 +move_rstp__(rstp);
  } else {
 -VLOG_DBG(%s, port %u: Bad BPDU received, p-rstp-name,
 +VLOG_DBG(%s, port %u: Bad RSTP BPDU received, p-rstp-name,
   p-port_number);
  p-error_count++;
  }
 The received BPDU could also be a STP BPDU.

 Changed the language here to “Bad STP or RSTP BPDU received.
 + * reference for that pointer.  This is OK as we never move
 + * ports from one bridge to another, and holders always
 + * release their ports before releasing the bridge.  This
 + * means that there should be not ports at this time. */
 +ovs_assert(rstp-ports_count == 0);
 Each RSTP port points back
 Thanks.

 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp,
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +
  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);

 These setters are the same in rstp_create() and reinitialize_rstp__(). We
 could define a funcion like rstp_initialize_port_defaults__() for the
 bridge.

 I will look into this.
 +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
 +OVS_REQUIRES(rstp_mutex)
  {
 -struct rstp *rstp;
 +/* XXX: Should we also support setting this to false, i.e., when port
 + * configuration is changed? */
 +if (mcheck == true  port-rstp-force_protocol_version = 2) {
 +port-mcheck = true;
 802.1D-2004 standard claims mcheck to be set from management and cleared
 from its procedure.
 *17.19.13 mcheck*
 *A boolean. May be set by management to force the Port Protocol Migration
 state machine to transmit RST*
 *BPDUs for a MigrateTime (17.13.9) period, to test whether all STP Bridges
 (17.4) on the attached LAN*
 *have been removed and the Port can continue to transmit RSTP BPDUs.
 Setting mcheck has no effect if*
 *stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP
 Compatibility” mode.*
 However to use it twice, I need to reset it in the database (to make it
 change when i want to invoke its setter), so i use the command with 0, with
 the only purpouse to clear it in the db, no action is needed from rstp.
 Then i can set it again and trigger the procedure.

 Removed the comment and added a note to this effect
 to vswitchd/vswitch.xml.

  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add here
 this line:
 xport-rstp_port = rstp_port;

 I don’t seem to be able to reproduce the segfault. I tried this by adding
 ovs-vsctl del-br and del-port commands to the new test cases, and they
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is
 

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-09 Thread Jarno Rajahalme

On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com 
wrote:

 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I plan 
 to squash trivial CodingStyle fixes in before pushing the series to master. 
 Also, I’ll add a News item stating that experimental RSTP is added, and more 
 compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.

Thanks!

 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working with an 
 IXIA validation software for a couple of weeks now, and I already have some 
 patches.
 I'm still solving some problems and I expect to have some more fixes. This 
 should take a couple of weeks to obtain a compliant version, so maybe it 
 would be better to wait for a compliant version of the protocol before 
 merging it to the master. Anyway, if you still want to merge it and mark it 
 as experimental is still fine by me, since I'll be able to rebase my patches 
 on your version.
 

Nice to hear about the IXIA validation work. I’ll merge the series to master 
soon, so that we have less different versions around.

 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 I looked and applied the patches. They’re good to me, I just have some notes 
 on patch 13/18 and 16/18.
 

(snip)

 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the bridge.
  
 I will look into this.

Later.

(snip)

  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add here 
 this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by adding 
 ovs-vsctl del-br and del-port commands to the new test cases, and they 
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. I 
 see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct xport 
 *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.

Good :-)

  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void 
 *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = rstp_port_ref(rstp_port);
 +}
 @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
  if (ofport-may_enable != enable) {
  struct ofproto_dpif *ofproto = 
 ofproto_dpif_cast(ofport-up.ofproto);
 -ofproto-backer-need_revalidate = REV_PORT_TOGGLED;
 -}
 -ofport-may_enable = enable;
 +ofproto-backer-need_revalidate = REV_PORT_TOGGLED;
 -if (ofport-rstp_port) {
 -if (rstp_port_get_mac_operational(ofport-rstp_port) != enable) {
 +   

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-09 Thread Daniele Venturino

Il giorno 09/set/2014, alle ore 20:07, Jarno Rajahalme jrajaha...@nicira.com 
ha scritto:

 
 On Sep 9, 2014, at 3:53 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 
 
 Thanks for the review!
 It would be nice to have an Acked-by from you to the series. However, I plan 
 to squash trivial CodingStyle fixes in before pushing the series to master. 
 Also, I’ll add a News item stating that experimental RSTP is added, and more 
 compliance and interoperability testing is needed.
 Responses below,
   Jarno
 
 Ok. I sent my acks.
 
 Thanks!
 
 
 I did some minor changes from June to August, that you can see here: 
 https://github.com/M3S/ovs-rstp/compare/b946221...c910081
 
 About the compliance and interoperability testing, I've been working with an 
 IXIA validation software for a couple of weeks now, and I already have some 
 patches.
 I'm still solving some problems and I expect to have some more fixes. This 
 should take a couple of weeks to obtain a compliant version, so maybe it 
 would be better to wait for a compliant version of the protocol before 
 merging it to the master. Anyway, if you still want to merge it and mark it 
 as experimental is still fine by me, since I'll be able to rebase my patches 
 on your version.
 
 
 Nice to hear about the IXIA validation work. I’ll merge the series to master 
 soon, so that we have less different versions around.

Thanks to you for your reviews! I’ll let you have my other patches soon.

 
 
 On Sep 3, 2014, at 7:44 AM, Daniele Venturino venturino.dani...@gmail.com 
 wrote:
 I looked and applied the patches. They’re good to me, I just have some notes 
 on patch 13/18 and 16/18.
 
 
 (snip)
 
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, 
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the bridge.
  
 I will look into this.
 
 Later.
 
 (snip)
 
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 I get a segfault when removing a port from a bridge. I don't if I add here 
 this line:
 xport-rstp_port = rstp_port;
 
 I don’t seem to be able to reproduce the segfault. I tried this by adding 
 ovs-vsctl del-br and del-port commands to the new test cases, and they 
 succeed just fine.
 The code right below will set the rstp_port member, if the new value is 
 different from the old value. So all the line you added above does is 
 circumvent the unref of the old pointer, and the ref of the new pointer. I 
 see that I missed the unref in xlate_xport_remove():
 diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
 index dd9536c..425b171 100644
 --- a/ofproto/ofproto-dpif-xlate.c
 +++ b/ofproto/ofproto-dpif-xlate.c
 @@ -932,6 +932,7 @@ xlate_xport_remove(struct xlate_cfg *xcfg, struct xport 
 *xport)
  hmap_remove(xport-xbridge-xports, xport-ofp_node);
  
  netdev_close(xport-netdev);
 +rstp_port_unref(xport-rstp_port);
  cfm_unref(xport-cfm);
  bfd_unref(xport-bfd);
  free(xport);
 Could you check if this resolves the segfault you get?
 
 It appears to be solved.
 
 Good :-)
 
  
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void 
 *bpdu, size_t bpdu_size)
 /* Each RSTP port poits back to struct rstp without holding a
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +static void
 static void
 
 
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = rstp_port_ref(rstp_port);
 +}
 @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
  if (ofport-may_enable != enable) {
  struct ofproto_dpif *ofproto = 
 ofproto_dpif_cast(ofport-up.ofproto);
 -ofproto-backer-need_revalidate = REV_PORT_TOGGLED;
 -}
 -

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-08 Thread Jarno Rajahalme
Thanks for the review!

It would be nice to have an Acked-by from you to the series. However, I plan to 
squash trivial CodingStyle fixes in before pushing the series to master. Also, 
I’ll add a News item stating that experimental RSTP is added, and more 
compliance and interoperability testing is needed.

Responses below,

  Jarno

On Sep 3, 2014, at 7:44 AM, Daniele Venturino venturino.dani...@gmail.com 
wrote:

 I looked and applied the patches. They’re good to me, I just have some notes 
 on patch 13/18 and 16/18.
 
 
 @@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void 
 *bpdu, size_t bpdu_size)
  memcpy(p-received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
  rstp-changes = true;
 -move_rstp(rstp);
 +move_rstp__(rstp);
  } else {
 -VLOG_DBG(%s, port %u: Bad BPDU received, p-rstp-name,
 +VLOG_DBG(%s, port %u: Bad RSTP BPDU received, p-rstp-name,
   p-port_number);
  p-error_count++;
  }
 
 The received BPDU could also be a STP BPDU.
 

Changed the language here to “Bad STP or RSTP BPDU received.

 /* Each RSTP port poits back to struct rstp without holding a
 + * reference for that pointer.  This is OK as we never move
 + * ports from one bridge to another, and holders always
 + * release their ports before releasing the bridge.  This
 + * means that there should be not ports at this time. */
 +ovs_assert(rstp-ports_count == 0);
 
 Each RSTP port points back

Thanks.

 
  +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);
 
 
 These setters are the same in rstp_create() and reinitialize_rstp__(). We 
 could define a funcion like rstp_initialize_port_defaults__() for the bridge.
  

I will look into this.

 +static void
 +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
 +OVS_REQUIRES(rstp_mutex)
  {
 -struct rstp *rstp;
 +/* XXX: Should we also support setting this to false, i.e., when port
 + * configuration is changed? */
 +if (mcheck == true  port-rstp-force_protocol_version = 2) {
 +port-mcheck = true;
 
 802.1D-2004 standard claims mcheck to be set from management and cleared from 
 its procedure.
 
 17.19.13 mcheck
 A boolean. May be set by management to force the Port Protocol Migration 
 state machine to transmit RST
 BPDUs for a MigrateTime (17.13.9) period, to test whether all STP Bridges 
 (17.4) on the attached LAN
 have been removed and the Port can continue to transmit RSTP BPDUs. Setting 
 mcheck has no effect if
 stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP 
 Compatibility” mode.
 
 However to use it twice, I need to reset it in the database (to make it 
 change when i want to invoke its setter), so i use the command with 0, with 
 the only purpouse to clear it in the db, no action is needed from rstp. Then 
 i can set it again and trigger the procedure.
 

Removed the comment and added a note to this effect to vswitchd/vswitch.xml.

 
 static void
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state 
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;
 
 I get a segfault when removing a port from a bridge. I don't if I add here 
 this line:
 xport-rstp_port = rstp_port;
 

I don’t seem to be able to reproduce the segfault. I tried this by adding 
ovs-vsctl del-br and del-port commands to the new test cases, and they succeed 
just fine.

The code right below will set the rstp_port member, if the new value is 
different from the old value. So all the line you added above does is 
circumvent the unref of the old pointer, and the ref of the new pointer. I see 
that I missed the unref in xlate_xport_remove():

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index dd9536c..425b171 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c

Re: [ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-09-03 Thread Daniele Venturino
I looked and applied the patches. They’re good to me, I just have some
notes on patch 13/18 and 16/18.


@@ -108,9 +121,9 @@  process_received_bpdu(struct rstp_port *p, const void
 *bpdu, size_t bpdu_size)
  memcpy(p-received_bpdu_buffer, bpdu, sizeof(struct rstp_bpdu));
  rstp-changes = true;
 -move_rstp(rstp);
 +move_rstp__(rstp);
  } else {
 -VLOG_DBG(%s, port %u: Bad BPDU received, p-rstp-name,
 +VLOG_DBG(%s, port %u: Bad RSTP BPDU received, p-rstp-name,
   p-port_number);
  p-error_count++;
  }


The received BPDU could also be a STP BPDU.

/* Each RSTP port poits back to struct rstp without holding a
 + * reference for that pointer.  This is OK as we never move
 + * ports from one bridge to another, and holders always
 + * release their ports before releasing the bridge.  This
 + * means that there should be not ports at this time. */
 +ovs_assert(rstp-ports_count == 0);


Each RSTP port points back

 +rstp_set_bridge_priority__(rstp, RSTP_DEFAULT_PRIORITY);
 +rstp_set_bridge_ageing_time__(rstp, RSTP_DEFAULT_AGEING_TIME);
 +rstp_set_bridge_forward_delay__(rstp,
 RSTP_DEFAULT_BRIDGE_FORWARD_DELAY);
 +rstp_set_bridge_hello_time__(rstp);
 +rstp_set_bridge_max_age__(rstp, RSTP_DEFAULT_BRIDGE_MAX_AGE);
 +rstp_set_bridge_migrate_time__(rstp);
 +rstp_set_bridge_transmit_hold_count__(rstp,
 +
  RSTP_DEFAULT_TRANSMIT_HOLD_COUNT);
 +rstp_set_bridge_times__(rstp, RSTP_DEFAULT_BRIDGE_FORWARD_DELAY,
 +RSTP_BRIDGE_HELLO_TIME,
 +RSTP_DEFAULT_BRIDGE_MAX_AGE, 0);


These setters are the same in rstp_create() and reinitialize_rstp__(). We
could define a funcion like rstp_initialize_port_defaults__() for the
bridge.


 +static void
 +rstp_port_set_mcheck__(struct rstp_port *port, bool mcheck)
 +OVS_REQUIRES(rstp_mutex)
  {
 -struct rstp *rstp;
 +/* XXX: Should we also support setting this to false, i.e., when port
 + * configuration is changed? */
 +if (mcheck == true  port-rstp-force_protocol_version = 2) {
 +port-mcheck = true;


802.1D-2004 standard claims mcheck to be set from management and cleared
from its procedure.

*17.19.13 mcheck*
*A boolean. May be set by management to force the Port Protocol Migration
state machine to transmit RST*
*BPDUs for a MigrateTime (17.13.9) period, to test whether all STP Bridges
(17.4) on the attached LAN*
*have been removed and the Port can continue to transmit RSTP BPDUs.
Setting mcheck has no effect if*
*stpVersion (17.20.12) is TRUE, i.e., the Bridge is operating in “STP
Compatibility” mode.*

However to use it twice, I need to reset it in the database (to make it
change when i want to invoke its setter), so i use the command with 0, with
the only purpouse to clear it in the db, no action is needed from rstp.
Then i can set it again and trigger the procedure.


static void
  xlate_xport_set(struct xport *xport, odp_port_t odp_port,
  const struct netdev *netdev, const struct cfm *cfm,
 -const struct bfd *bfd, int stp_port_no, int rstp_port_no,
 +const struct bfd *bfd, int stp_port_no,
 +const struct rstp_port* rstp_port,
  enum ofputil_port_config config, enum ofputil_port_state
 state,
  bool is_tunnel, bool may_enable)
  {
  xport-config = config;
  xport-state = state;
  xport-stp_port_no = stp_port_no;
 -xport-rstp_port_no = rstp_port_no;


I get a segfault when removing a port from a bridge. I don't if I add here
this line:
xport-rstp_port = rstp_port;


 xport-is_tunnel = is_tunnel;
  xport-may_enable = may_enable;
  xport-odp_port = odp_port;
 +if (xport-rstp_port != rstp_port) {
 +rstp_port_unref(xport-rstp_port);
 +xport-rstp_port = rstp_port_ref(rstp_port);
 +}
 @@ -3133,16 +3088,15 @@  port_run(struct ofport_dpif *ofport)
  if (ofport-may_enable != enable) {
  struct ofproto_dpif *ofproto =
 ofproto_dpif_cast(ofport-up.ofproto);
 -ofproto-backer-need_revalidate = REV_PORT_TOGGLED;
 -}
 -ofport-may_enable = enable;
 +ofproto-backer-need_revalidate = REV_PORT_TOGGLED;
 -if (ofport-rstp_port) {
 -if (rstp_port_get_mac_operational(ofport-rstp_port) != enable) {
 +if (ofport-rstp_port) {
  rstp_port_set_mac_operational(ofport-rstp_port, enable);
  }
  }
 +
 +ofport-may_enable = enable;
  }


rstp_port_set_mac_operational(ofport-rstp_port, enable) should be outside
 if (ofport-may_enable != enable) otherwise ports remain disabled when
added.


In patch 16/18:

diff --git a/lib/rstp-state-machines.c b/lib/rstp-state-machines.c
 index e8b8438..5ae7124 100644
 --- a/lib/rstp-state-machines.c
 +++ b/lib/rstp-state-machines.c
 @@ -2015,42 +2015,33 @@  compare_rstp_priority_vector(struct
 rstp_priority_vector 

[ovs-dev] [PATCH v6 00/18] Rapid Spanning Tree Protocol

2014-08-20 Thread Jarno Rajahalme
I took my time starting the review, so I decided address issues as I
see them rather than just comment on them.

The first patch of this series is a minimally rebased version of the
v5 sent on ovs-dev on June 12th, 2014.  Rest of the series is my
proposal for fixes and enhancements.

I could have reordered and squashed some of the patches together, but
that would have been more work...

Daniele Venturino (1):
  Rapid Spanning Tree Protocol (IEEE 802.1D).

Jarno Rajahalme (17):
  lib/stp,rstp: Add unit more unit tests.
  lib/stp: Some debugging support.
  lib/rstp: Better debug messages, style fixes.
  vswitch.xml: Fix RSTP configuration documentation.
  lib/rstp: Remove unused struct rstp_priority_vector4
  lib/rstp: Coding style fixes.
  lib/rstp: Refactor priority vector recalculation.
  lib/rstp: Refactor port number allocation.
  lib/rstp: Refactor port initialization.
  lib/rstp: CodingStyle changes.
  lib/rstp: Inline trivial predicate functions.
  lib/rstp: More robust thread safety.
  lib/rstp: Remove lock recursion.
  lib/rstp: CodingStyle fixes.
  lib/rstp: Simplify priority vector comparison.
  lib/rstp: Eliminate ports_count.
  lib/rstp: Use hmap instead of a list for ports.

 AUTHORS  |1 +
 NOTICE   |3 +
 lib/automake.mk  |5 +
 lib/packets.h|5 +
 lib/rstp-common.h|  879 ++
 lib/rstp-state-machines.c| 2025 ++
 lib/rstp-state-machines.h|   46 +
 lib/rstp.c   | 1369 
 lib/rstp.h   |  290 ++
 lib/stp.c|7 +-
 lib/stp.h|5 -
 ofproto/ofproto-dpif-xlate.c |  157 +++-
 ofproto/ofproto-dpif-xlate.h |6 +-
 ofproto/ofproto-dpif.c   |  255 +-
 ofproto/ofproto-provider.h   |   47 +
 ofproto/ofproto.c|   84 ++
 ofproto/ofproto.h|   50 ++
 tests/.gitignore |1 +
 tests/automake.mk|3 +
 tests/ovs-vsctl.at   |4 +
 tests/rstp.at|  235 +
 tests/stp.at |  100 +++
 tests/test-rstp.c|  714 +++
 tests/testsuite.at   |1 +
 utilities/ovs-vsctl.8.in |   79 ++
 vswitchd/bridge.c|  295 ++
 vswitchd/vswitch.ovsschema   |   15 +-
 vswitchd/vswitch.xml |  131 ++-
 28 files changed, 6749 insertions(+), 63 deletions(-)
 create mode 100644 lib/rstp-common.h
 create mode 100644 lib/rstp-state-machines.c
 create mode 100644 lib/rstp-state-machines.h
 create mode 100644 lib/rstp.c
 create mode 100644 lib/rstp.h
 create mode 100644 tests/rstp.at
 create mode 100644 tests/test-rstp.c

-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev