[ovs-discuss] [PATCH] lib/ofp-util.c, lib/vconn.c, lib/rconn.c, lib/ofp-errors.c:: OFPERR_OFPBRC_BAD_VERSION generated from switch when there is version mismatch.

[Ben Pfaff]

"git am" says:

    /home/blp/nicira/ovs/.git/rebase-apply/patch:9: trailing whitespace.
        if (version != type->ofp_version) {                                    
    /home/blp/nicira/ovs/.git/rebase-apply/patch:10: trailing whitespace.
            return OFPERR_OFPBRC_BAD_VERSION;                                  
    /home/blp/nicira/ovs/.git/rebase-apply/patch:24: trailing whitespace.
                    retval = OFPERR_OFPBRC_BAD_VERSION;                            
    /home/blp/nicira/ovs/.git/rebase-apply/patch:25: trailing whitespace.
                    return retval;                                               
    /home/blp/nicira/ovs/.git/rebase-apply/patch:60: trailing whitespace.
        if (error == OFPERR_OFPBRC_BAD_VERSION) {                                          
    warning: squelched 2 whitespace errors
    warning: 7 lines add whitespace errors.

and in fact you added tons of trailing whitespace.

The following change doesn't make any sense for at least two reasons.
First, there's already a version check a couple of lines above.  Second,
this is along an error path, where 'type' points beyond the end of an
array:
> @@ -355,6 +355,9 @@ ofputil_lookup_openflow_message(const struct ofputil_msg_category *cat,
>          }
>      }
>  
> +    if (version != type->ofp_version) {                                    
> +        return OFPERR_OFPBRC_BAD_VERSION;                                  
> +    }
>      VLOG_WARN_RL(&bad_ofmsg_rl, "received %s of unknown type %"PRIu32,
>                   cat->name, value);
>      return cat->missing_error;

The following change leaks memory and causes vconn_recv() to return an
OFPERR_* whereas it's documented to return an errno value:

> diff --git a/lib/vconn.c b/lib/vconn.c
> index 5da5026..47622c6 100644
> --- a/lib/vconn.c
> +++ b/lib/vconn.c
> @@ -555,6 +555,8 @@ do_recv(struct vconn *vconn, struct ofpbuf **msgp)
>                              "%s: received OpenFlow version 0x%02"PRIx8" "
>                              "!= expected %02x",
>                              vconn->name, oh->version, vconn->version);
> +                retval = OFPERR_OFPBRC_BAD_VERSION;                            
> +                return retval;                                               
>              }
>              ofpbuf_delete(*msgp);
>              retval = EPROTO;
> 

The following forces all OFPERR_OFPBRC_BAD_VERSION errors to be encoded
in OpenFlow 1.0.  That's not acceptable given that we're actively
working on OF1.1+ support:

> --- a/lib/ofp-errors.c
> +++ b/lib/ofp-errors.c
> @@ -187,6 +187,9 @@ ofperr_encode_reply(enum ofperr error, const struct ofp_header *oh)
>      uint16_t len = ntohs(oh->length);
>  
>      domain = ofperr_domain_from_version(oh->version);
> +    if (error == OFPERR_OFPBRC_BAD_VERSION) {                                          
> +        domain = ofperr_domain_from_version(OFP10_VERSION);                       
> +    }                                                                            
>      return ofperr_encode_msg__(error, domain, oh->xid, oh, MIN(len, 64));
>  }

Try again?