Thanks for the reviews, I'll do another round of unit tests and push
this.
On Fri, Jan 04, 2013 at 12:39:19PM -0800, Ethan Jackson wrote:
> Acked-by: Ethan Jackson <et...@nicira.com>
>
>
> On Wed, Jan 2, 2013 at 5:10 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> > rconn_add_monitor() tries to check the version of the controller
> > connection being monitored, so that it can decide what OpenFlow version to
> > tell the monitor connection to negotiate. But at any given time an rconn
> > may not have a controller connection (e.g. during backoff), so rc->vconn
> > may be null and thus vconn_get_version(rc->vconn) dereferences a null
> > pointer.
> >
> > Fixing the problem in a local way would require the rconn to remember the
> > previous version negotiated, and that fails if the rconn hasn't yet
> > connected or if the next connection negotiates a new version.
> >
> > This commit instead adds the ability to a vconn to accept any OpenFlow
> > message version and modifies "ovs-ofctl snoop" to use that feature, thus
> > removing the need to negotiate the "correct" version on snoops.
> >
> > Bug #14265.
> > Reported-by: Pratap Reddy <pre...@nicira.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > lib/rconn.c | 8 --------
> > lib/vconn-provider.h | 13 +++++++++----
> > lib/vconn.c | 23 ++++++++++++++++++++++-
> > lib/vconn.h | 8 ++++++--
> > utilities/ovs-ofctl.c | 18 ++++++++++++------
> > 5 files changed, 49 insertions(+), 21 deletions(-)
> >
> > diff --git a/lib/rconn.c b/lib/rconn.c
> > index 7a2bcf5..1ea022b 100644
> > --- a/lib/rconn.c
> > +++ b/lib/rconn.c
> > @@ -674,14 +674,6 @@ void
> > rconn_add_monitor(struct rconn *rc, struct vconn *vconn)
> > {
> > if (rc->n_monitors < ARRAY_SIZE(rc->monitors)) {
> > - int version = vconn_get_version(rc->vconn);
> > -
> > - /* Override the allowed versions of the snoop vconn so that
> > - * only the version of the controller connection is allowed.
> > - * This is because the snoop will see the same messages as the
> > - * controller */
> > - vconn_set_allowed_versions(vconn, 1u << version);
> > -
> > VLOG_INFO("new monitor connection from %s",
> > vconn_get_name(vconn));
> > rc->monitors[rc->n_monitors++] = vconn;
> > } else {
> > diff --git a/lib/vconn-provider.h b/lib/vconn-provider.h
> > index a26d9c5..c92eb63 100644
> > --- a/lib/vconn-provider.h
> > +++ b/lib/vconn-provider.h
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2008, 2009, 2010, 2012 Nicira, Inc.
> > + * Copyright (c) 2008, 2009, 2010, 2012, 2013 Nicira, Inc.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -33,13 +33,18 @@ struct vconn {
> > struct vconn_class *class;
> > int state;
> > int error;
> > - uint32_t allowed_versions;
> > - uint32_t peer_versions;
> > - enum ofp_version version;
> > +
> > + /* OpenFlow versions. */
> > + uint32_t allowed_versions; /* Bitmap of versions we will accept. */
> > + uint32_t peer_versions; /* Peer's bitmap of versions it will
> > accept. */
> > + enum ofp_version version; /* Negotiated version (or 0). */
> > + bool recv_any_version; /* True to receive a message of any
> > version. */
> > +
> > ovs_be32 remote_ip;
> > ovs_be16 remote_port;
> > ovs_be32 local_ip;
> > ovs_be16 local_port;
> > +
> > char *name;
> > };
> >
> > diff --git a/lib/vconn.c b/lib/vconn.c
> > index a3792ec..6f318a9 100644
> > --- a/lib/vconn.c
> > +++ b/lib/vconn.c
> > @@ -395,6 +395,27 @@ vconn_get_version(const struct vconn *vconn)
> > return vconn->version ? vconn->version : -1;
> > }
> >
> > +/* By default, a vconn accepts only OpenFlow messages whose version
> > matches the
> > + * one negotiated for the connection. A message received with a different
> > + * version is an error that causes the vconn to drop the connection.
> > + *
> > + * This functions allows 'vconn' to accept messages with any OpenFlow
> > version.
> > + * This is useful in the special case where 'vconn' is used as an rconn
> > + * "monitor" connection (see rconn_add_monitor()), that is, where 'vconn'
> > is
> > + * used as a target for mirroring OpenFlow messages for debugging and
> > + * troubleshooting.
> > + *
> > + * This function should be called after a successful vconn_open() or
> > + * pvconn_accept() but before the connection completes, that is, before
> > + * vconn_connect() returns success. Otherwise, messages that arrive on
> > 'vconn'
> > + * beforehand with an unexpected version will the vconn to drop the
> > + * connection. */
> > +void
> > +vconn_set_recv_any_version(struct vconn *vconn)
> > +{
> > + vconn->recv_any_version = true;
> > +}
> > +
> > static void
> > vcs_connecting(struct vconn *vconn)
> > {
> > @@ -601,7 +622,7 @@ vconn_recv(struct vconn *vconn, struct ofpbuf **msgp)
> > if (!retval) {
> > retval = do_recv(vconn, &msg);
> > }
> > - if (!retval) {
> > + if (!retval && !vconn->recv_any_version) {
> > const struct ofp_header *oh = msg->data;
> > if (oh->version != vconn->version) {
> > enum ofptype type;
> > diff --git a/lib/vconn.h b/lib/vconn.h
> > index 81bdcc9..b15388c 100644
> > --- a/lib/vconn.h
> > +++ b/lib/vconn.h
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
> > + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -38,14 +38,18 @@ int vconn_open(const char *name, uint32_t
> > allowed_versions, uint8_t dscp,
> > struct vconn **vconnp);
> > void vconn_close(struct vconn *);
> > const char *vconn_get_name(const struct vconn *);
> > +
> > uint32_t vconn_get_allowed_versions(const struct vconn *vconn);
> > void vconn_set_allowed_versions(struct vconn *vconn,
> > uint32_t allowed_versions);
> > +int vconn_get_version(const struct vconn *);
> > +void vconn_set_recv_any_version(struct vconn *);
> > +
> > ovs_be32 vconn_get_remote_ip(const struct vconn *);
> > ovs_be16 vconn_get_remote_port(const struct vconn *);
> > ovs_be32 vconn_get_local_ip(const struct vconn *);
> > ovs_be16 vconn_get_local_port(const struct vconn *);
> > -int vconn_get_version(const struct vconn *);
> > +
> > int vconn_connect(struct vconn *);
> > int vconn_recv(struct vconn *, struct ofpbuf **);
> > int vconn_send(struct vconn *, struct ofpbuf *);
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index bc2773e..96c4741 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -368,21 +368,23 @@ open_vconn_socket(const char *name, struct vconn
> > **vconnp)
> > return error;
> > }
> >
> > +enum open_target { MGMT, SNOOP };
> > +
> > static enum ofputil_protocol
> > -open_vconn__(const char *name, const char *default_suffix,
> > +open_vconn__(const char *name, enum open_target target,
> > struct vconn **vconnp)
> > {
> > + const char *suffix = target == MGMT ? "mgmt" : "snoop";
> > char *datapath_name, *datapath_type, *socket_name;
> > enum ofputil_protocol protocol;
> > char *bridge_path;
> > int ofp_version;
> > int error;
> >
> > - bridge_path = xasprintf("%s/%s.%s", ovs_rundir(), name,
> > default_suffix);
> > + bridge_path = xasprintf("%s/%s.%s", ovs_rundir(), name, suffix);
> >
> > ofproto_parse_name(name, &datapath_name, &datapath_type);
> > - socket_name = xasprintf("%s/%s.%s",
> > - ovs_rundir(), datapath_name, default_suffix);
> > + socket_name = xasprintf("%s/%s.%s", ovs_rundir(), datapath_name,
> > suffix);
> > free(datapath_name);
> > free(datapath_type);
> >
> > @@ -399,6 +401,10 @@ open_vconn__(const char *name, const char
> > *default_suffix,
> > ovs_fatal(0, "%s is not a bridge or a socket", name);
> > }
> >
> > + if (target == SNOOP) {
> > + vconn_set_recv_any_version(*vconnp);
> > + }
> > +
> > free(bridge_path);
> > free(socket_name);
> >
> > @@ -421,7 +427,7 @@ open_vconn__(const char *name, const char
> > *default_suffix,
> > static enum ofputil_protocol
> > open_vconn(const char *name, struct vconn **vconnp)
> > {
> > - return open_vconn__(name, "mgmt", vconnp);
> > + return open_vconn__(name, MGMT, vconnp);
> > }
> >
> > static void
> > @@ -1456,7 +1462,7 @@ ofctl_snoop(int argc OVS_UNUSED, char *argv[])
> > {
> > struct vconn *vconn;
> >
> > - open_vconn__(argv[1], "snoop", &vconn);
> > + open_vconn__(argv[1], SNOOP, &vconn);
> > monitor_vconn(vconn);
> > }
> >
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
