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 <b...@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.
I noticed a compile time warning.
This is different to the warning I reported in a separate email.
[snip]
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
[snip]
> @@ -2975,12 +2417,18 @@ ofputil_put_switch_features_port(const struct
> ofputil_phy_port *pp,
> /* Decodes the OpenFlow "port status" message in '*ops' into an abstract form
> * in '*ps'. Returns 0 if successful, otherwise an OFPERR_* value. */
> enum ofperr
> -ofputil_decode_port_status(const struct ofp_port_status *ops,
> +ofputil_decode_port_status(const struct ofp_header *oh,
> struct ofputil_port_status *ps)
> {
> + const struct ofp_port_status *ops;
> + enum ofpraw raw;
> struct ofpbuf b;
> int retval;
>
> + ofpbuf_use_const(&b, oh, ntohs(oh->length));
> + raw = ofpraw_pull_assert(&b);
> + ops = ofpbuf_pull(&b, sizeof *ops);
> +
> if (ops->reason != OFPPR_ADD &&
> ops->reason != OFPPR_DELETE &&
> ops->reason != OFPPR_MODIFY) {
The variable raw seems to be unused in this function,
perhaps it could be removed?
> @@ -2988,9 +2436,7 @@ ofputil_decode_port_status(const struct ofp_port_status
> *ops,
> }
> ps->reason = ops->reason;
>
> - ofpbuf_use_const(&b, ops, ntohs(ops->header.length));
> - ofpbuf_pull(&b, sizeof *ops);
> - retval = ofputil_pull_phy_port(ops->header.version, &b, &ps->desc);
> + retval = ofputil_pull_phy_port(oh->version, &b, &ps->desc);
> assert(retval != EOF);
> return retval;
> }
[snip]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
