On Fri, May 06, 2016 at 08:16:16PM +0530, Numan Siddique wrote:
> This patch adds a new OVN action 'dhcp_offer' to support native
> DHCP in OVN.
>
> 'dhcp_offer' takes the DHCP options as input params.
> Eg. dhcp_offer(offerip = 10.0.0.4, router = 10.0.0.1,
> netmask = 255.255.255.0, lease_time = 3600,....)
>
> ovn-controller parses this action and adds a NXT_PACKET_IN2
> OF flow with 'pause' flag set and the DHCP options stored in
> 'userdata' field.
>
> When the DHCP packet is received by ovn-controller, it frames a
> new DHCP reply packet with the DHCP options present in the
> 'userdata' field and resumes the packet.
>
> Eg. dhcp_offer(offerip = 10.0.0.4, router = 10.0.0.1,
> netmask = 255.255.255.0, lease_time = 3600,....)
>
> A new 'DHCP_Options' table is added in SB DB which stores
> the support DHCP options with DHCP code and type. ovn-northd is
> expected to popule this table.
>
> The next patch will add logical flows with this action.
>
> Signed-Off-by: Numan Siddique <nusid...@redhat.com>
Thanks for sending v4!
pinctrl_handle_dhcp_offer() is not very skeptical about the contents of
buffers. I think that it should do checking, e.g.:
ovs_be32 *offer_ip = ofpbuf_try_pull(userdata, sizeof *offer_ip);
if (!offer_ip) {
/* XXX */
}
if (dp_packet_l4_size(pkt) < UDP_HEADER_LEN + sizeof (struct dhcp_header) +
sizeof(uint32_t) + 3) {
/* XXX */
}
The DHCP parsing looked totally bizarre before I re-read the DHCP RFC
and re-discovered that in fact the DHCP type is 6 bytes past the DHCP
header for compatibility with BOOTP. A comment might help.
These casts shouldn't be needed:
dhcp_options[0] = (uint8_t) DHCP_OPT_MSG_TYPE;
dhcp_options[1] = (uint8_t) 1;
It would probably be better to use names that are less easily confused
than 'pkt' for the input and 'packet' for the output. 'in' and 'out',
'src' and 'dst', 'old' and 'new', whatever, I don't care much as long as
they're distinct..
It's risky to assume that the L2 header in the input packet is exactly
ETH_HEADER_LEN bytes (what if there's a VLAN? or an MPLS header?) and
that the L3 header is exactly IP_HEADER_LEN bytes. The input packet has
l2 and l3 pointers and lengths, please use them (including in the IP
total length and csum calculations).
Why zero the IP TOS field?
Oh, also s/pinctl/pinctrl/ in the function name.
GCC points out some format specifiers that need updates:
diff --git a/tests/test-ovn-dhcp.c b/tests/test-ovn-dhcp.c
index 60726d9..d55299b 100644
--- a/tests/test-ovn-dhcp.c
+++ b/tests/test-ovn-dhcp.c
@@ -102,7 +102,7 @@ test_dhcp_offer_action(int argc OVS_UNUSED, char *argv[]
OVS_UNUSED)
if (oc->userdata_len != (expected_dhcp_opts.size + sizeof *ah)) {
ovs_fatal(1, "Error. dhcp_offer parse action failed : userdata length"
- " mismatch. Expected - %lu : Actual - %u",
+ " mismatch. Expected - %"PRIuSIZE" : Actual - %"PRIu16,
expected_dhcp_opts.size + sizeof *ah, oc->userdata_len);
}
I think that the documentation could be a little more detailed. For
example, the action acts only on DHCPDISCOVER and DHCPREQUEST and does
nothing to non-DHCP or other kinds of DHCP. In fact, is the latter a
problem? I think that, currently, there is no way to detect that
nothing happened, so I think that the DHCP server will end up sending
the request back to the client as if it were a reply, which is really
weird behavior. I can imagine a few different ways to avoid that, e.g.:
- Have the action take a set of actions in {} that only get
executed if it really does something.
- Have the action set a field that only gets executed if it
really does something.
I'm also not sure that most people will be able to interpret the
documentation for the DHCP_Options table. Can you add a little more
exposition and refer to the RFCs that define options? Also a few
examples?
In particular I don't think that the DHCP_OPT_TYPE_* codes are anything
standardized. I think that it would make more sense to put them into
the database as "enum"s than as numbers, e.g. "bool", "uint8", ..., and
then explain in the documentation what these mean.
In parse_dhcp_option(), instead of using hard-coded offsets like
cs.values[0].value.u8[127], please use accessors, like
ntohll(cs.values[0].value.integer). Then nothing will break if the size
of mf_subvalue changes. Maybe we should add more accessors.
Please type-check the set in parse_dhcp_option(), that is, if the option
is an integer, check that 'cs' is an integer, and similar for strings.
Otherwise we could end up dereferencing a "pointer" that is actually an
integer.
In parse_dhcp_offer_action(), please free dhcp_opts on the error path
too. Actually, can you parse the options directly into the ofpacts like
this? I guess you'd need a special case for the offered IP, but there's
already a special case for that.
size_t oc_offset = start_controller_op(ctx->ofpacts,
ACTION_OPCODE_DHCP_OFFER, true);
while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
if (!parse_dhcp_option(ctx, ctx->ofpacts)) {
return;
}
}
finish_controller_op(ctx->ofpacts, oc_offset);
Thanks a lot!
Ben
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
