I think the handling should be uniform, if we need any at all.
On Fri, May 25, 2012 at 01:16:44PM +0000, Prabina Pattnaik wrote: > Hi Ben, > > This is grey out area of openflow 1.0 spec that with which request packet > "OFPBRC_BAD_VERSION" should come. > If we go with remaining request packet like ECHO_REQUEST, BARRIER_REQUEST and > GET_CONFIG_REQUEST, this error is not coming and connection is resetting in > case of BARRIER_REQUEST and GET_CONFIG_REQUEST. > > We haven't handled for above packets because controller is not supposed to > get any useful data in a data field from switch. > It can be supported for OFPBRC_BAD_VERSION error type to handle all request > packets to prevent the reset connection when there is version mismatch > between request and reply packet. > > Regards, > Prabina > > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Wednesday, May 23, 2012 10:11 PM > To: Prabina Pattnaik > Cc: [email protected]; Varun Gupta; [email protected] > Subject: Re: [ovs-discuss] [PATCH] ofproto/ofproto.c, lib/vconn.c:: > OFPBRC_BAD_VERSION generated from switch when there is version mismatch. > > You made changes to a few randomly selected OpenFlow messages. What > is special about these messages? Why change them and not others? > > On Wed, May 23, 2012 at 11:59:15AM +0000, Prabina Pattnaik wrote: > > Hi Ben, > > > > Design description and rationale - > > --------------------------------------- > > We are adding switch configuration message (OFPT_FEATURE_REQUEST) and > > statistics message (OFPT_STATS_REQUEST) as a condition in vconn.c file > > to prevent the reset of connection on receiving incorrect version in > > feature request and stat request packets from controller. > > Checking the version mismatch between stat request / feature request > > packets and version supported by switch in ofproto.c file. If versions > > are different then return the error packet to controller. > > > > Testing after merging patch in OVS 1.4.1 > > ------------------------------------------------- > > > > 1. Verify OFPET_BAD_REQUEST OFPBRC_BAD_VERSION code message from > > switch to controller for switch description statistics, aggregate > > statistics, > > table statistics, port statistics and queue statistics when there is > > mismatch version. > > 2. Verify stats request and reply message from controller to switch > > for switch description statistics, aggregate statistics, table statistics, > > > > port statistics and queue statistics when version match. > > 3. Verify OFPET_BAD_REQUEST OFPBRC_BAD_VERSION code message from switch > > to controller for feature request message when there is mismatch version. > > 4. Verify feature request and feature reply messages from controller to > > switch for switch description statistics, aggregate statistics, table > > statistics, > > flow statistics and queue statistics when version match. > > > > > > Patch > > --------- > > ofproto/ofproto.c | 21 +++++++++++++++++++++ > > lib/vconn.c | 4 +++- > > 2 files changed, 24 insertions(+), 1 deletions(-) > > > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > > index 9057215..0a7fbcc 100644 > > --- a/ofproto/ofproto.c > > +++ b/ofproto/ofproto.c > > @@ -2894,6 +2894,9 @@ handle_openflow__(struct ofconn *ofconn, const struct > > ofpbuf *msg) > > return handle_echo_request(ofconn, oh); > > > > case OFPUTIL_OFPT_FEATURES_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_features_request(ofconn, oh); > > > > case OFPUTIL_OFPT_GET_CONFIG_REQUEST: > > @@ -2933,23 +2936,41 @@ handle_openflow__(struct ofconn *ofconn, const > > struct ofpbuf *msg) > > > > /* Statistics requests. */ > > case OFPUTIL_OFPST_DESC_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_desc_stats_request(ofconn, msg->data); > > > > case OFPUTIL_OFPST_FLOW_REQUEST: > > case OFPUTIL_NXST_FLOW_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_flow_stats_request(ofconn, msg->data); > > > > case OFPUTIL_OFPST_AGGREGATE_REQUEST: > > case OFPUTIL_NXST_AGGREGATE_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_aggregate_stats_request(ofconn, msg->data); > > > > case OFPUTIL_OFPST_TABLE_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_table_stats_request(ofconn, msg->data); > > > > case OFPUTIL_OFPST_PORT_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_port_stats_request(ofconn, msg->data); > > > > case OFPUTIL_OFPST_QUEUE_REQUEST: > > + if (oh->version != OFP_VERSION) { > > + return ofp_mkerr(OFPET_BAD_REQUEST,OFPBRC_BAD_VERSION); > > + } > > return handle_queue_stats_request(ofconn, msg->data); > > > > case OFPUTIL_MSG_INVALID: > > > > diff --git a/lib/vconn.c b/lib/vconn.c > > index 6ea9366..c499623 100644 > > --- a/lib/vconn.c > > +++ b/lib/vconn.c > > @@ -546,7 +546,9 @@ do_recv(struct vconn *vconn, struct ofpbuf **msgp) > > && oh->type != OFPT_ERROR > > && oh->type != OFPT_ECHO_REQUEST > > && oh->type != OFPT_ECHO_REPLY > > - && oh->type != OFPT_VENDOR) > > + && oh->type != OFPT_VENDOR > > + && oh->type != OFPT_FEATURES_REQUEST > > + && oh->type != OFPT_STATS_REQUEST) > > { > > if (vconn->version < 0) { > > VLOG_ERR_RL(&bad_ofmsg_rl, > > > > Regards, > > Prabina > > > > -----Original Message----- > > From: [email protected] > > [mailto:[email protected]] On Behalf Of Ben Pfaff > > Sent: Tuesday, May 22, 2012 11:40 PM > > To: Varun Gupta > > Cc: [email protected] > > Subject: Re: [ovs-discuss] OpenVSwitch - "OFPBRC_BAD_VERSION" error packet > > not received when there is version mismatch between stats request and stats > > reply > > > > Can you provide a patch? I'll apply it, if it's reasonable. > > > > On Tue, May 22, 2012 at 07:40:49AM +0000, Varun Gupta wrote: > > > The two issues I raised (stats request/reply and feature request/reply) > > > were found when we were evaluating the switch's response in case the > > > controller behave indifferently. > > > Our objective is to make OVS more robust and respond with an error > > > (OFPBRC_BAD_VERSION) in the cases of 'OFPET_BAD_REQUEST' error type as > > > mentioned in spec (1.0): > > > > > > ------------------------------------------- > > > > > > For the OFPET_BAD_REQUEST error type, the following codes are currently > > > de- > > > fined: > > > /* ofp_error_msg 'code' values for OFPET_BAD_REQUEST. 'data' contains at > > > least > > > * the first 64 bytes of the failed request. */ > > > enum ofp_bad_request_code { > > > OFPBRC_BAD_VERSION, /* ofp_header.version not supported. */ > > > ---------- > > > ---------- > > > }; > > > > > > ------------------------------------------ > > > > > > Regards, > > > Varun > > > > > > -----Original Message----- > > > From: Ben Pfaff [mailto:[email protected]] > > > Sent: Monday, May 21, 2012 10:04 PM > > > To: Varun Gupta > > > Cc: [email protected] > > > Subject: Re: [ovs-discuss] OpenVSwitch - "OFPBRC_BAD_VERSION" error > > > packet not received when there is version mismatch between stats request > > > and stats reply > > > > > > On Mon, May 21, 2012 at 10:50:19AM +0000, Varun Gupta wrote: > > > > Switch is not notifying the controller for OFPBRC_BAD_VERSION, /* > > > > ofp_header.version not supported. */ when there is version mismatch > > > > between stats request and stats reply. The switch was resetting the > > > > connection and only a warning message was generated on switch in log > > > > (/var/log/messages) before resetting connection. > > > > > > It's hard for me to consider this a serious bug, because it's a bug > > > in the controller to send an OpenFlow message of a version that was > > > not negotiated in the OpenFlow version negotiation. I don't know why > > > you'd do that. > > > > > > > > > > > > DISCLAIMER: > > > > > > ----------------------------------------------------------------------------------------------------------------------- > > > > > > The contents of this e-mail and any attachment(s) are confidential and > > > intended > > > > > > for the named recipient(s) only. > > > > > > It shall not attach any liability on the originator or NECHCL or its > > > > > > affiliates. Any views or opinions presented in > > > > > > this email are solely those of the author and may not necessarily reflect > > > the > > > > > > opinions of NECHCL or its affiliates. > > > > > > Any form of reproduction, dissemination, copying, disclosure, > > > modification, > > > > > > distribution and / or publication of > > > > > > this message without the prior written consent of the author of this > > > e-mail is > > > > > > strictly prohibited. If you have > > > > > > received this email in error please delete it and notify the sender > > > > > > immediately. . > > > > > > ----------------------------------------------------------------------------------------------------------------------- > > _______________________________________________ > > discuss mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/discuss > > > > > > > > DISCLAIMER: > > > > ----------------------------------------------------------------------------------------------------------------------- > > > > The contents of this e-mail and any attachment(s) are confidential and > > intended > > > > for the named recipient(s) only. > > > > It shall not attach any liability on the originator or NECHCL or its > > > > affiliates. Any views or opinions presented in > > > > this email are solely those of the author and may not necessarily reflect > > the > > > > opinions of NECHCL or its affiliates. > > > > Any form of reproduction, dissemination, copying, disclosure, modification, > > > > distribution and / or publication of > > > > this message without the prior written consent of the author of this e-mail > > is > > > > strictly prohibited. If you have > > > > received this email in error please delete it and notify the sender > > > > immediately. . > > > > ----------------------------------------------------------------------------------------------------------------------- > > > > DISCLAIMER: > > ----------------------------------------------------------------------------------------------------------------------- > > The contents of this e-mail and any attachment(s) are confidential and > intended > > for the named recipient(s) only. > > It shall not attach any liability on the originator or NECHCL or its > > affiliates. Any views or opinions presented in > > this email are solely those of the author and may not necessarily reflect the > > opinions of NECHCL or its affiliates. > > Any form of reproduction, dissemination, copying, disclosure, modification, > > distribution and / or publication of > > this message without the prior written consent of the author of this e-mail is > > strictly prohibited. If you have > > received this email in error please delete it and notify the sender > > immediately. . > > ----------------------------------------------------------------------------------------------------------------------- _______________________________________________ discuss mailing list [email protected] http://openvswitch.org/mailman/listinfo/discuss
