networkd: Introduce geneve GEneric NEtwork Virtualization Encapsulation#5752
networkd: Introduce geneve GEneric NEtwork Virtualization Encapsulation#5752poettering merged 3 commits intosystemd:masterfrom
Conversation
poettering
left a comment
There was a problem hiding this comment.
I don't really know what GENEVE is, hence I only did a superficial code review, and found a number of type issues. Please be more careful with the types: always use the right type the kernel expects, and define parsers that refuse anything outside of the allowed range.
I really hope you tested all of this...
| GENEVE.Id, config_parse_uint32, 0, offsetof(Geneve, id) | ||
| GENEVE.Remote, config_parse_geneve_address, 0, offsetof(Geneve, remote) | ||
| GENEVE.TOS, config_parse_unsigned, 0, offsetof(Geneve, tos) | ||
| GENEVE.TTL, config_parse_unsigned, 0, offsetof(Geneve, ttl) |
There was a problem hiding this comment.
the man page you added says the range for this is 1-155 (is that true, isn't that a typo? should that be 255?). If so, then please add config_parse_geneve_ttl() or so, which ensures the value passed is in the right range.
|
|
||
| v->dest_port = port; | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
We should really generalize this into a generic config_parse_ip_port() or so which is added to conf-parser.h, and then used wherever we parse IP ports, which I figure is a non-zero number of places already. Any chance you can make this happen and rework the users of parse_ip_port()?
There was a problem hiding this comment.
Yes I am thinking of making it a separate patch.
src/network/netdev/geneve.h
Outdated
| struct Geneve { | ||
| NetDev meta; | ||
|
|
||
| uint64_t id; |
There was a problem hiding this comment.
hu? is this 32bit or 64bit? You aprse this with config_parse_uint32(), but define the type to be uint64_t... that looks very wrong..
src/network/netdev/geneve.h
Outdated
| union in_addr_union remote; | ||
|
|
||
| unsigned tos; | ||
| unsigned ttl; |
There was a problem hiding this comment.
the kernel accepts this as u8, hence this should have the type uint8_t here, too
| </varlistentry> | ||
| <varlistentry> | ||
| <term><varname>TOS=</varname></term> | ||
| <listitem> |
There was a problem hiding this comment.
is this an integer? if so, say so. what's the valid range for it?
| VXLAN.FlowLabel, config_parse_flow_label, 0, 0 | ||
| GENEVE.Id, config_parse_uint32, 0, offsetof(Geneve, id) | ||
| GENEVE.Remote, config_parse_geneve_address, 0, offsetof(Geneve, remote) | ||
| GENEVE.TOS, config_parse_unsigned, 0, offsetof(Geneve, tos) |
There was a problem hiding this comment.
please use a proper parser function for this, and generate a parsing error log msg for invalid values
src/network/netdev/geneve.h
Outdated
|
|
||
| unsigned tos; | ||
| unsigned ttl; | ||
| unsigned flow_label; |
There was a problem hiding this comment.
the kernel wants this as uint32_t... please use the right type here too
src/network/netdev/geneve.h
Outdated
|
|
||
| union in_addr_union remote; | ||
|
|
||
| unsigned tos; |
There was a problem hiding this comment.
please use uint8_t here, if that's what the kernel expects
This work enables cration of geneve tunnel
| return 0; | ||
| } | ||
|
|
||
| v->tos = f; |
There was a problem hiding this comment.
hmm, this function is entirely generic, and accepts all kinds of uint8_t, hence I think we should just introduce config_parse_uint8() or so as part of conf-parser.[ch], similar in style to the existing config_parse_uint16(), config_parse_uint32() and we have and use that.
|
OK, looks good to me. But I'd really appreciate a follow-up commit that adds generic config_parse_uint8() and config_prase_ip_port() functions and uses then for this stuff and elsewhere, see comments. |
This work enables cration of geneve tunnel.