On 12/23/21 14:08, Eelco Chaudron wrote:
Just some small comments below, and the request to fix up the comments.
Thanks.
Trying to understand why you have a special ODPFlowFactory class to return an
ODPFLow() object from a string?
Can you not just add a static method to the ODPFlow class, so it would simply
become a matter of calling:
ODPFlow.from_string(string, id=id_data)
Or probably more OOO is to override the __init__ function of ODPFlow() to take
only the string and the id.
Don’t think there is a use case to call ODPFlow() with sections and raw.
I'll implement decoders caching via static class variables in the next version.
+class ODPFlowFactory:
+ """Datapath Flow"""
+
+ def __init__(self):
+ self.info_decoders = self._info_decoders()
+ self.match_decoders = self._match_decoders()
+ self.action_decoders = self._action_decoders()
+
+ def from_string(self, odp_string, id=None):
+ """Parse a odp flow string
+
+ The string is expected to have the follwoing format:
+ [ufid], [match] [flow data] actions:[actions]
+
+ Args:
+ odp_string (str): a datapath flow string
+
+ Returns:
+ an ODPFlow instance
+ """
+
+ sections = []
+
+ # If UFID present, parse it and
+ ufid_pos = odp_string.find("ufid:")
+ if ufid_pos >= 0:
+ ufid_string = odp_string[
+ ufid_pos : (odp_string[ufid_pos:].find(",") + 1)
+ ]
+ ufid_parser = KVParser(KVDecoders({"ufid": decode_default}))
+ ufid_parser.parse(ufid_string)
+ if len(ufid_parser.kv()) != 1:
+ raise ValueError("malformed odp flow: %s" % odp_string)
+ sections.append(
+ Section("ufid", ufid_pos, ufid_string, ufid_parser.kv())
+ )
+
+ action_pos = odp_string.find("actions:")
+ if action_pos < 0:
+ raise ValueError("malformed odp flow: %s" % odp_string)
+
+ # rest of the string is between ufid and actions
+ rest = odp_string[
+ (ufid_pos + len(ufid_string) if ufid_pos >= 0 else 0) : action_pos
+ ]
+
+ action_pos += 8 # len("actions:")
+ actions = odp_string[action_pos:]
+
+ field_parts = rest.lstrip(" ").partition(" ")
+
+ if len(field_parts) != 3:
+ raise ValueError("malformed odp flow: %s" % odp_string)
+
+ match = field_parts[0]
+ info = field_parts[2]
+
+ iparser = KVParser(KVDecoders(self.info_decoders))
+ iparser.parse(info)
Looking at the two lines above, to me, it looks like the object might not have
been defined right (but I’m not an OOO expert).
I would have designed it as, KVParser(string, decoders=None), this way you
would have a single object initialization, so:
parser = KVParser(info, KVDecoders(self.info_decoders))
Also because the parse() method does not allow re-use of the object, meaning
parse another string with the same instance.
Or was there a specific reason to split this in two stages?
It's a remainder of a previous design. Right now I agree it doesn't make sense.
I'll change it in the next version.
+ isection = Section(
+ name="info",
+ pos=odp_string.find(info),
+ string=info,
+ data=iparser.kv(),
+ )
+ sections.append(isection)
+
+ mparser = KVParser(KVDecoders(self.match_decoders))
+ mparser.parse(match)
+ msection = Section(
+ name="match",
+ pos=odp_string.find(match),
+ string=match,
+ data=mparser.kv(),
+ )
+ sections.append(msection)
+
+ aparser = KVParser(
+ KVDecoders(self.action_decoders, default_free=decode_free_output)
+ )
+ aparser.parse(actions)
+ asection = Section(
+ name="actions",
+ pos=action_pos,
+ string=actions,
+ data=aparser.kv(),
+ is_list=True,
+ )
+ sections.append(asection)
+
+ return ODPFlow(sections, odp_string, id)
+
+ @classmethod
Any reason why all of these are @classmethod, and not @staticmethod, as they do
not seem to need a reference to self?
Guess this is also true in some of the previous patches, but I did not notice ;)
Right.
[...]
+
+ @classmethod
+ def _field_decoders(cls):
+ return {
+ "skb_priority": Mask32,
+ "skb_mark": Mask32,
+ "recirc_id": decode_int,
+ "dp_hash": Mask32,
+ "ct_state": decode_default, # TODO: Parse flags
Guess we might want to fix this before the merge?
Well, I've left it initially to confirm whether there is a strong need for this.
Decoding ct_state with a integer allows you to set any ct_state, only you have
to do it using the hexadecimal representation.
Do you think we'll want to use string representation of flags? Same goes for
other flags like tcp_flags.
[...]
+def decode_geneve(mask, value):
+ """
+ Decode geneve options. Used for both tnl_push(header(geneve(options())))
+ action and tunnel(geneve()) match.
+
+ It has the following format:
+
+ {class=0xffff,type=0x80,len=4,0xa}
+
+ Args:
+ mask (bool): Whether masking is supported
+ value (str): The value to decode
+ """
+ if mask:
Create some consistency around the ending of doctext and the first line of code
(in all patches).
Sometimes you have an empty line, like in decode_tnl_gre below, and sometimes
not like here.
For some reason I was expecting my formatter to do that for me, but I guess it's
not... Sure, I'll double check.
--
Adrián Moreno
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
