On 12/24/21 15:07, Eelco Chaudron wrote:
+class OFPFlowFactory:
See my comments on patch 8, where I do think we should get rid of this class,
and update the OFPFlow class to take this string at init.
Being more OOO it would look like this:
def __init__(self, ofp_string, id=None):
"""Constructor"""
sections = self._sections_from_string(ofp_string)
super(OFPFlow, self).__init__(sections, ofp_string, id)
Where sections_from_string(ofp_string) is just the same code as in this class
but returns the sections.
@staticmethod
def _sections_from_string(ofp_string):
...
That was more or less how I originally implemented it. I then refactored it into
a factory class because when parsing thousands of flows I saw a considerable
amount of time being spent on parser "preparation", this is: creating all the
KVParser objects. So I created the factory just to cache this data.
Another alternative would be to make them global (for the class) and initialize
them the first time we parse a flow so we could keep the nicer syntax. I'm
trying to avoid running functions on module load. WDYT?
Do we need OFPFlowFactory(object):, some times flake reports this as H238. But
might no longer be valid with the latest python3.
Not 100% sure either, I'll check. Thanks.
+ """OpenFlow Flow Factory is a class capable of creating OFPFLow objects"""
+
+ def __init__(self):
+ self.info_decoders = self._info_decoders()
+ self.match_decoders = KVDecoders(
The name suggests that we return KVDecoders() as the __info_decoders() does, I
think all three should return the same.
What name exactly? Do you mean the _flow_match_decoders and _field_decoders?
Or do you mean match_decoders should be a function rather than the object
directly?
Trying to remember what my thoughts were ;) Guess I wondered why
_flow_match_decoders() was not doing the ‘return
KVDecoders({**self._field_decoders(), **self._flow_match_decoders()})’.
I'll unify the names to make them more consistent.
[...]
+
+ @classmethod
+ def _output_actions_decoders(cls):
+ """Returns the decoders for the output actions"""
If you have them, I think it might be useful, to add some example strings to
the individual decoders. This way, it's easy to see what they intend to decode.
Ack.
[...]
+def decode_learn(action_decoders):
It’s getting late, and I have a hard time (focussing ;) understanding where the
value for this one comes from? I'll pick it up from here when I continue the
review.
I hope I can clarify. Learn action has two added complexities:
- It accepts any other action key-value. For this we need to create a wrapper
around the pre-calculated action_decoders.
- The way fields can be specified is augmented. Not only we have 'field=value', but we
also have 'field=_src_' (where _src_ is another field name) and just 'field'. For this we
need to create a wrapper of field_decoders that, for each "field=X" key-value
we check if X is a field_name or if it's acually a value that we need to send to the
appropriate field_decoder to process.
Ack thanks, it makes sense now.
I'll add a comment to make it clearer.
--
Adrián Moreno
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
