On 11 September 2015 at 14:02, Jarno Rajahalme <jrajaha...@nicira.com> wrote: >>> - new and reply are mutually exclusive (reply direction is defined as >>> opposite of the initial +new) >> >> Strictly speaking I don't think this is the case. I don't have an >> example, but it is possible for this to be set in the conntrack info >> specified in the Linux interface, see >> linux/netfilter/nf_conntrack_common.h. ct_state is translated roughly >> from this and potentially other pieces of conntrack information. >> > > While IP_CT_NEW_REPLY is defined, it is only used by > openvswitch/connntrack.c, and the next line says “(no NEW in reply dirn.)” > > So it looks like IP_CT_NEW_REPLY should not even be defined. I have prepared > a patch to remove the IP_CT_NEW_REPLY definition, but have no immediate plans > to send it out. > > As said, it seems to me that reply direction is defined as the opposing > direction to the packet that initially created the conntrack entry. I’m still > a bit confused about related vs. related_reply, though.
Fair enough. I can describe this constraint in the documentation. I don't have an answer for related_reply right now, but I can investigate and tell you whether the "reply" refers to the original connection or the related connection. >>> - related and new can be set at the same time (but does related remain set >>> for the duration of the connection, or is it also mutually exclusive with >>> established?) >> >> Related can be present with any other bit. I believe that it remains >> set for the duration of the connection. Looks like a good candidate >> for another test in the testsuite. >> > > ctinfo enum can not hold related and established at the same time, though. True, but the ovs key can (and does). Each time we perform the ct lookup, we translate the state, then, if there is a related (ct->master) connection, set the related bit. >>> - apparently related and reply are not mutually exclusive, but when are >>> they set at the same time? >> >> I see "related" as another bit that is independent of all of the >> others; whether the connection is related or not, the packets can >> still be part of a new connection, established, reply, etc. Perhaps >> the more pressing question is exactly what "reply" means in this >> context: Is it referring to the direction of the original connection, >> or only the related connection? This is something I can further >> clarify. >> > > in ctinfo enum related, new, and established are all mutually exclusive. In > OVS ct_state this is not always the case, though, as we explicitly set the > new bit for related, too. I agree that it would be nice to be able to persist > the related bit also when the connection state is “established”. I agree, and I'm pretty sure this is what we do already (but I don't have a test to explicitly check that this is the behaviour). >>>> + * sent through the connection tracker, and pipeline processing for the >>>> cloned >>>> + * packet will continue in the OpenFlow table specified by this value. The >>>> + * NXM_NX_CT_* fields will be populated for the subsequent processing. It >>>> is >>>> + * strongly recommended that this table is later than the current table, >>>> to >>>> + * prevent loops. >>>> + * >>> >>> ‘alg’ and ofpacts are introduced in later patches, and I guess it is too >>> much work to not deal with them here yet? >> >> I could go back and adjust the definition to place padding there and >> drop the ofpacts instead. > > Seems like a lot of work, though, essentially only for reviewing purposes. Up > to you :-) Guess why I didn't do it in the first place :-) >>> What about the version? >> >> When decoding the CT action, we have lost the context of which >> OpenFlow version we are decoding. So we can't determine whether the >> nested actions are valid for the current OpenFlow version. Having >> version=OFP10_VERSION has worked in all of the cases I have tried so >> far, perhaps because the nicira extensions extend all openflow >> versions. I'm not sure if there's a more correct way to apply this, or >> if passing OFP10_VERSION is sufficient here. Thoughts? >> > > Not sure. Maybe test with each OpenFlow version and see what happens? I also tried OFP11_VERSION, and for the tests I have, there is no difference. Maybe it's entirely incidental. I haven't followed the code closely enough to discover any potential pitfalls. Perhaps this could just be highlighted, eg at the top of the function, something like: int version = OFP10_VERSION; /* As this is an nx-action, all features are available so OFP10_VERSION is sufficient to parse all possible nested actions. */ >> This is "coincidental". Strictly speaking the OpenFlow wire format for >> these flags should be translated into an internal representation, >> which should be subsequently translated into the equivalent datapath >> representation, but in practice they're all the same. Perhaps this >> would be more explicit to define them using the netlink interface >> definitions: >> >> #define CS_NEW OVS_CS_F_NEW >> …. > > Would the current code work if the mapping was not 1:1? If yes, then it does > not matter. If not, then we should at least assert that the mapping stays 1:1 > also in the future. No; the state is copied directly from the nx_action_conntrack -> ofpact_conntrack -> nla_put_*(). Adding an assert sounds prudent, I'll do this. > Btw. I got the “make check-kernel” run successfully for this patch :-) Great, hoping to spread understanding of this feature so people can build more datapath tests :D _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
