[ovs-dev] [PATCH 04/18] ofp-util: Add version bitmap to hello messages

Simon Horman horms at verge.net.au
Thu Oct 25 00:55:06 UTC 2012


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 <horms at 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.



More information about the dev mailing list