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

[Simon Horman]

On Thu, Jul 19, 2012 at 11:28:47PM -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>

I have run into a lot of this code while porting my Open Flow 1.2 patches,
which I estimate I am now about half way though. I don't have any
objections to anything I see here.

As discussed elsewhere[1] I think that lib/ofp-msgs.c needs to be slightly
enhanced in order to handle OpenFlow 1.2, so assuming the changes you
indicated you will roll into this patch are rolled in.

Reviewed-by: Simon Horman <horms at verge.net.au>

[1] elsewhere: http://openvswitch.org/pipermail/dev/2012-July/019442.html
    "Re: [PATCH 05/24] ofp-util: Allow decoder to use OFPT11_STATS_{REQUEST, REPLY} for OpenFlow 1.2"

> ---
> 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.
> 
> v5->v6: Rebased against master (more work than you might guess).
> Fixed bugs in extract-ofp-msgs reported by Simon Horman.
> ---
>  build-aux/extract-ofp-msgs         |  351 +++++++++
>  include/openflow/nicira-ext.h      |  117 +---
>  include/openflow/openflow-1.0.h    |   74 +--
>  include/openflow/openflow-1.1.h    |   83 +--
>  include/openflow/openflow-1.2.h    |   23 +-
>  include/openflow/openflow-common.h |   75 +--
>  lib/.gitignore                     |    1 +
>  lib/automake.mk                    |    9 +
>  lib/learning-switch.c              |  128 ++--
>  lib/ofp-errors.c                   |   41 +-
>  lib/ofp-errors.h                   |    4 +-
>  lib/ofp-msgs.c                     |  962 +++++++++++++++++++++++++
>  lib/ofp-msgs.h                     |  433 ++++++++++++
>  lib/ofp-print.c                    |  321 +++++----
>  lib/ofp-util.c                     | 1372 ++++++------------------------------
>  lib/ofp-util.h                     |  134 +----
>  lib/rconn.c                        |   72 ++-
>  lib/stream.c                       |    4 +-
>  lib/vconn.c                        |   68 +-
>  ofproto/connmgr.c                  |   24 +-
>  ofproto/ofproto.c                  |  228 +++----
>  tests/ofp-print.at                 |    2 +-
>  tests/ofproto-dpif.at              |    1 +
>  tests/ofproto-macros.at            |    2 +-
>  tests/test-vconn.c                 |   68 +-
>  utilities/ovs-ofctl.c              |  176 +++---
>  26 files changed, 2669 insertions(+), 2104 deletions(-)

I know that breaking up is hard to do,
but something shy of 5000 lines with a bonus 1000 lines of generated code
is a big patch to review.