[ovs-dev] [of1.1 v5 1/5] ofp-msgs: New approach to encoding and decoding OpenFlow headers.

Ben Pfaff blp at nicira.com
Thu Jul 19 17:55:24 UTC 2012


On Thu, Jul 19, 2012 at 05:14:57PM +0900, Isaku Yamahata wrote:
> On Thu, Jul 19, 2012 at 04:31:38PM +0900, Simon Horman wrote:
> > On Thu, Jul 05, 2012 at 11:12:29PM -0700, Ben Pfaff wrote:
> > > OpenFlow headers are not as uniform as they could be, with size, alignment,
> > > and numbering changes from one version to another and across varieties
> > > (e.g. ordinary messages vs. "stats" messages).  Until now the Open vSwitch
> > > internal APIs haven't done a good job of abstracting those differences in
> > > header formats.  This commit changes that; from this commit forward very
> > > little code actually needs to understand the header format or numbering.
> > > Instead, it can just encode or decode, or pull or put, the header using
> > > a more abstract API using the ofpraw_, ofptype_, and other APIs in the
> > > new ofp-msgs module.
> > > 
> > > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > > ---
> > > v3: New commit.  This isn't complete yet: in particular the program for
> > > extracting the numbers from the ofp-msgs.h header file isn't written,
> > > although there's a skeleton.  That means that nothing has been tested or
> > > carefully looked over, either.
> > > 
> > > v3->v4: Almost ready.  The ofp-msgs.c functions need some comments
> > > and careful review.
> > > 
> > > v4->v5: Ready for review.
> > 
> > Not a proper review, but I noticed a compile-time warning.
> > 
> > [snip]
> > 
> > > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > > index 7b0b220..425448e 100644
> > 
> > [snip]
> > 
> > > @@ -620,18 +608,21 @@ fetch_port_by_stats(const char *vconn_name,
> > >          run(vconn_recv_block(vconn, &reply), "OpenFlow packet receive failed");
> > >          recv_xid = ((struct ofp_header *) reply->data)->xid;
> > >          if (send_xid == recv_xid) {
> > > -            const struct ofputil_msg_type *type;
> > > -            struct ofp_stats_msg *osm;
> > > -
> > > -            ofputil_decode_msg_type(reply->data, &type);
> > > -            if (ofputil_msg_type_code(type) != OFPUTIL_OFPST_PORT_DESC_REPLY) {
> > > +            struct ofp_header *oh;
> > > +            enum ofptype type;
> > > +            struct ofpbuf b;
> > > +            uint16_t flags;
> > > +
> > > +            ofpbuf_use_const(&b, oh, ntohs(oh->length));
> > 
> > gcc tells me that oh is used uninitialised here and that does seem
> > to be the case to me too.
> > 
> > Should it be initialised to ((struct ofp_header *) reply->data) ?
> 
> How about this patch?
> 
> commit 3da23e05619d5b21b057b20dc078ca03e522da4c
> Author: Isaku Yamahata <yamahata at valinux.co.jp>
> Date:   Thu Jul 19 17:13:33 2012 +0900
> 
>     ovs-ofctl: uninitialized variable oh
>     
>     This patch fixes the following warning.
>     
>     > openvswitch/utilities/ovs-ofctl.c: In function 'fetch_ofputil_phy_port':
>     > openvswitch/utilities/ovs-ofctl.c:616:89: warning: 'oh' may be used uninitialized in this function [-Wuninitialized]
>     
>     Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
> 
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 7bf8e0f..b6679f7 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -608,7 +608,7 @@ fetch_port_by_stats(const char *vconn_name,
>          run(vconn_recv_block(vconn, &reply), "OpenFlow packet receive failed");
>          recv_xid = ((struct ofp_header *) reply->data)->xid;
>          if (send_xid == recv_xid) {
> -            struct ofp_header *oh;
> +            struct ofp_header *oh = reply->data;
>              enum ofptype type;
>              struct ofpbuf b;
>              uint16_t flags;
> 
> 
> -- 
> yamahata

Thanks, but I'd prefer to just fix it in the initial commit rather
than with a followup patch.



More information about the dev mailing list