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

[Simon Horman]

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) ?


> +            if (ofptype_pull(&type, &b)
> +                || type != OFPTYPE_PORT_DESC_STATS_REPLY) {
>                  ovs_fatal(0, "received bad reply: %s",
>                            ofp_to_string(reply->data, reply->size,
>                                          verbosity + 1));
>              }
>  
> -            osm = ofpbuf_at_assert(reply, 0, sizeof *osm);
> -            done = !(ntohs(osm->flags) & OFPSF_REPLY_MORE);
> +            flags = ofpmp_flags(oh);
> +            done = !(flags & OFPSF_REPLY_MORE);
>  
>              if (found) {
>                  /* We've already found the port, but we need to drain

[ snip ]