On Wed, Oct 24, 2012 at 10:14:30AM -0700, Ben Pfaff wrote: > On Thu, Oct 18, 2012 at 02:58:04PM +0900, Simon Horman wrote: > > Allow encoding and decoding of version bitmap in hello messages > > as specified in Open Flow 1.3.1. > > > > Signed-off-by: Simon Horman <ho...@verge.net.au> > > Thanks for doing this. > > It looks like ofputil_decode_hello() only processes a single hello > element, only if that one is an OFPHET_VERSIONBITMAP element. OF1.3.1 > says: > > Implementations must ignore (skip) all elements of a Hello message > that they do not support. > > so I'd be inclined to say that, instead, we should iterate over each > element that is present, looking for an OFPHET_VERSIONBITMAP element and > silently ignoring any others that we find.
Good point. I will add that logic. > It looks like the declarations of ofputil_hello, ofputil_decode_hello(), > and ofputil_encode_hello() should be moved from patch 2 to this patch, > because I don't see any reference to them before this patch. Sorry, I think that is just an artifact of my patch shuffling. I'll clean that up. > I'm not certain that ofputil_hello actually needs the 'version' member. > To decode a hello message without a bitmap, one can initialize the > bitmap as every version up to the one specified; to decode a hello > message with a bitmap, one initializes the bitmap from the included > bitmap. To encode a hello message given only a bitmap is equally > straightforward. (And if I'm right about all that, then maybe we don't > need a struct at all--perhaps a single uint32_t bitmap is sufficient?) I had considered that and I am happy with that approach. The reason I decided on the implementation with a separate version number is to allow ofp_print_hello() to omit printing the bitmap if it wasn't in the hello message on the wire. However, this is arguably cosmetic and always using a bitmap would simplify several code paths. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
