On 13 Jan 2022, at 12:33, Adrian Moreno wrote:
> 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.
Good question!
Here is a dump with -m (ovs-appctl dpctl/dump-flows -m):
ufid:97fc770e-2233-4b72-bfbc-7dbf3870ddf5,
recirc_id(0x1),dp_hash(0/0),skb_priority(0/0),in_port(ovsp0),skb_mark(0/0),ct_state(0x1/0x1),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/00:00:00:00:00:00),eth_type(0x0800,ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
packets:0, bytes:0, used:never, dp:ovs,
actions:ct(commit,zone=1,mark=0x5/0x5,label=0x5/0x5),ct(commit,zone=2),ovs-p1
And one without:
recirc_id(0x1),in_port(2),ct_state(+new),eth(),eth_type(0x0800),ipv4(proto=6,frag=no),
packets:0, bytes:0, used:never,
actions:ct(commit,zone=1,mark=0x5/0x5,label=0x5/0x5),ct(commit,zone=2),3
So which one do we need!? Guess we need to be able to decode both, depending on
the input? Ping me offline if we need a quick brainstorming session.
> [...]
>>> +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
