On Thu, May 2, 2013 at 12:36 PM, Andy Zhou <az...@nicira.com> wrote: > This patch refactors the netlink flow handling code to make > the processing, error checking and prerequisite checking > more stream lined. > > Signed-off-by: Andy Zhou <az...@nicira.com>
I think separating parsing from validation is a good idea and will help a lot when we have both values and masks. I might even go a step further and move the checks for illegal values to the validation code so that we can share the exact same parsing code later on. The main thing that caught my eye was the handling of attributes in the presence of an encap tag. It looks to me like if there is an encap attribute then we will lose the information for anything not encapsulated because attrs will get overwritten. This could certainly be solved by not zeroing it out and just continuing to fill in bits (similar to what is being done with 'a'), which would likely work because the use of encap is pretty limited at this point in time. However, it seems somewhat error prone and not easily extensible in the future. I don't quite understand the benefit in using a loop plus a switch statement to parse the attributes versus a series of if statements. The latter seems easier to read to me since it has a more linear structure similar to the protocol layers and I believe that it would also avoid the above problem by parsing unencapsulated attributes before extracting the inner values. I also think that it would make it easier to check duplicate writes to the same fields as we extract them, which feels more natural to do in the initial phase rather than in final validation. This is especially true since when we add masks we'll want to check for duplicate writes there as well. I'm also not a huge fan of tracking the number of writes to each byte - I think it works but it feels a little overly heavyweight. Finally, I would provide a wrapper function to handle the initialization, parsing, and validation steps. I think all of the callers in datapath.c need to call these steps and it's likely to get even more complex later on. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
