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. .
>
> -----------------------------------------------------------------------------------------------------------------------
_______________________________________________
discuss mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/discuss
