| Message ID | 56B1D88C.4040703@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Feb 03, 2016 at 04:08:04PM +0530, Numan Siddique wrote: > After the commit ad99e2e, setting the switch config 'miss_send_len' > is failing in ovn-controller because of validation in OFPT_SET_CONFIG > messages. > > Signed-off-by: Numan Siddique <nusiddiq@redhat.com> This is an improvement in any case, because it makes better use of the OpenFlow abstractions we have. However, the problem you report probably indicates a bug somewhere else, because the OFPT_SET_CONFIG message is generated from the OFPT_GET_CONFIG_REPLY sent by the switch. If the switch rejects the OFPT_SET_CONFIG, then, it could mean that the OFPT_GET_CONFIG_REPLY that it generated was invalid. If true, I'd like to fix that. Can you tell me how to see the error? Plain "make sandbox SANDBOXFLAGS=--ovn" didn't make it manifest, for me. Thanks, Ben.
On 02/06/2016 03:01 AM, Ben Pfaff wrote: > However, the problem you report probably indicates a bug somewhere else, > because the OFPT_SET_CONFIG message is generated from the > OFPT_GET_CONFIG_REPLY sent by the switch. If the switch rejects the > OFPT_SET_CONFIG, then, it could mean that the OFPT_GET_CONFIG_REPLY that > it generated was invalid. If true, I'd like to fix that. Can you tell > me how to see the error? Plain "make sandbox SANDBOXFLAGS=--ovn" didn't > make it manifest, for me. I am able to reproduce the issue by running the below commands $make sandbox SANDBOXFLAGS="--ovn" $ovn/env1/setup.sh $ovs-ofctl add-flow br-int "table=0, priority=200, in_port=2, action=controller" $src_mac=000000000002 $request=ffffffffffff${src_mac}080045100110000000008011000000000000ffffffff0044004300FC0000 $ovs-appctl ofproto/trace br-int in_port=2 $request Before calling set_switch_config(swconn, &config) in ovn/controller/pinctrl.c at L108 (https://github.com/openvswitch/ovs/blob/master/ovn/controller/pinctrl.c#L108), If I set config.flags = 0; ovn-controller is getting the packet-in message from the Open vSwitch. Thanks Numan
On Sat, Feb 06, 2016 at 03:45:29PM +0530, Numan Siddique wrote: > On 02/06/2016 03:01 AM, Ben Pfaff wrote: > > However, the problem you report probably indicates a bug somewhere else, > > because the OFPT_SET_CONFIG message is generated from the > > OFPT_GET_CONFIG_REPLY sent by the switch. If the switch rejects the > > OFPT_SET_CONFIG, then, it could mean that the OFPT_GET_CONFIG_REPLY that > > it generated was invalid. If true, I'd like to fix that. Can you tell > > me how to see the error? Plain "make sandbox SANDBOXFLAGS=--ovn" didn't > > make it manifest, for me. > > I am able to reproduce the issue by running the below commands > > $make sandbox SANDBOXFLAGS="--ovn" > $ovn/env1/setup.sh > $ovs-ofctl add-flow br-int "table=0, priority=200, in_port=2, action=controller" > $src_mac=000000000002 > $request=ffffffffffff${src_mac}080045100110000000008011000000000000ffffffff0044004300FC0000 > $ovs-appctl ofproto/trace br-int in_port=2 $request > > > Before calling set_switch_config(swconn, &config) in ovn/controller/pinctrl.c at L108 > (https://github.com/openvswitch/ovs/blob/master/ovn/controller/pinctrl.c#L108), If I set > config.flags = 0; ovn-controller is getting the packet-in message from the Open vSwitch. Thanks. I found the bug and fixed it in a commit that I inserted just before yours. There wasn't any point in getting it reviewed because your commit then deletes the code, so I just went ahead and applied both to master.
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c index 360f38b..847731d 100644 --- a/ovn/controller/pinctrl.c +++ b/ovn/controller/pinctrl.c @@ -62,14 +62,11 @@ get_switch_config(struct rconn *swconn) } static void -set_switch_config(struct rconn *swconn, const struct ofp_switch_config *config) +set_switch_config(struct rconn *swconn, + const struct ofputil_switch_config *config) { - struct ofpbuf *request; - - request = - ofpraw_alloc(OFPRAW_OFPT_SET_CONFIG, rconn_get_version(swconn), 0); - ofpbuf_put(request, config, sizeof *config); - + enum ofp_version version = rconn_get_version(swconn); + struct ofpbuf *request = ofputil_encode_set_config(config, version); queue_msg(request); } @@ -98,12 +95,9 @@ pinctrl_recv(struct controller_ctx *ctx, const struct ofp_header *oh, if (type == OFPTYPE_ECHO_REQUEST) { queue_msg(make_echo_reply(oh)); } else if (type == OFPTYPE_GET_CONFIG_REPLY) { - struct ofpbuf rq_buf; - struct ofp_switch_config *config_, config; + struct ofputil_switch_config config; - ofpbuf_use_const(&rq_buf, oh, ntohs(oh->length)); - config_ = ofpbuf_pull(&rq_buf, sizeof *config_); - config = *config_; + ofputil_decode_get_config_reply(oh, &config); config.miss_send_len = htons(UINT16_MAX); set_switch_config(swconn, &config); } else if (type == OFPTYPE_PACKET_IN) {
After the commit ad99e2e, setting the switch config 'miss_send_len' is failing in ovn-controller because of validation in OFPT_SET_CONFIG messages. Signed-off-by: Numan Siddique <nusiddiq@redhat.com> --- ovn/controller/pinctrl.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)