On Mon, Jul 16, 2012 at 07:38:39PM +1200, Joe Stringer wrote: > On 13 July 2012 16:01, Ben Pfaff <b...@nicira.com> wrote: > > ...I think that we should restrict parsing NXAST_WRITE_METADATA to one > > entry and only as the final entry. ... > > There are only two important paths into ofpacts: parsing of binary > > instructions and actions, and parsing of text versions of instructions > > and actions. We'd want to make sure that both of these paths enforce > > the constraint. > > Would it just be enough to to ofpacts_check() to verify there's only > one ofpact_metadata, and it's at the end of 'ofpacts'? > > This seems like the appropriate way -- keeping it simple, and applying > to all versions of the protocol.. and it follows the comments for > ofpacts_pull_openflow10(),11() which say that it is the caller's > responsibility to check that the ofpacts are valid. My only concern is > that the other code which uses these pull functions don't appear use > this method to verify the ofpacts are valid.
Hmm. The comments don't express it, but the current intent is different. Each of the two paths into ofpacts is supposed to check the actions to the limit of the extent that it can. But some ofpacts require additional context to be fully checked (either the maximum port number or an associated flow). That context isn't always available at the time of parsing (and sometimes it's not a single context, since a set of actions might be getting applied to a whole bunch of flows at once), so we reserve that checking for the ofpacts_check() function. I don't really like that we need ofpacts_check(), and my tendency is to want to get rid of it somehow (not sure how), not to expand its scope. We should clarify the comments to make all of the above clear. I'll do that. So I'd rather check in the individual places. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
