Thanks Ansis for the review. Just FYI - I just sent v5 patchset yesterday
- https://patchwork.ozlabs.org/patch/622521/
- https://patchwork.ozlabs.org/patch/622522/
Please see the comments inline.
On Tue, May 17, 2016 at 12:15 AM, Ansis Atteka <ansisatt...@gmail.com>
wrote:
> On 6 May 2016 at 07:46, Numan Siddique <nusid...@redhat.com> 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 working on this. I did not try to apply your patch yet so I
> might have missed something while reviewing. Also, I was trying to
> recollect some DHCP behavioral quirks that I may or may not recollect
> correctly after my experience with DHCP. So feel free to ask me to
> collaborate on things that I mentioned here but don't make sense to
> you.
> > ---
> > lib/dhcp.h | 13 ++++
> > ovn/controller/lflow.c | 11 ++++
> > ovn/controller/pinctrl.c | 103 +++++++++++++++++++++++++++++++-
> > ovn/lib/actions.c | 151
> ++++++++++++++++++++++++++++++++++++++++++++++-
> > ovn/lib/actions.h | 8 +++
> > ovn/lib/automake.mk | 3 +-
> > ovn/lib/expr.c | 48 ++++-----------
> > ovn/lib/expr.h | 37 ++++++++++++
> > ovn/lib/ovn-dhcp.h | 121 +++++++++++++++++++++++++++++++++++++
> > ovn/ovn-sb.ovsschema | 13 +++-
> > ovn/ovn-sb.xml | 44 ++++++++++++++
> > tests/automake.mk | 1 +
> > tests/ovn.at | 39 ++++++++++++
> > tests/test-ovn-dhcp.c | 117 ++++++++++++++++++++++++++++++++++++
> > 14 files changed, 667 insertions(+), 42 deletions(-)
> > create mode 100644 ovn/lib/ovn-dhcp.h
> > create mode 100644 tests/test-ovn-dhcp.c
> >
> > diff --git a/lib/dhcp.h b/lib/dhcp.h
> > index 6f97298..271e0a5 100644
> > --- a/lib/dhcp.h
> > +++ b/lib/dhcp.h
> > @@ -25,6 +25,8 @@
> > #define DHCP_SERVER_PORT 67 /* Port used by DHCP server. */
> > #define DHCP_CLIENT_PORT 68 /* Port used by DHCP client. */
> >
> > +#define DHCP_MAGIC_COOKIE 0x63825363
> > +
> > #define DHCP_HEADER_LEN 236
> > struct dhcp_header {
> > uint8_t op; /* DHCP_BOOTREQUEST or DHCP_BOOTREPLY.
> */
> > @@ -45,4 +47,15 @@ struct dhcp_header {
> > };
> > BUILD_ASSERT_DECL(DHCP_HEADER_LEN == sizeof(struct dhcp_header));
> >
> > +#define DHCP_OP_REQUEST 1
> > +#define DHCP_OP_REPLY 2
> > +
> > +#define DHCP_MSG_DISCOVER 1
> > +#define DHCP_MSG_OFFER 2
> > +#define DHCP_MSG_REQUEST 3
> > +#define DHCP_MSG_ACK 5
> I don't see code where you would send DHCP NACK message. The typical
> use case for DHCPNACK is if DHCP server configuration changed at the
> serverside and DHCP client state machine should immediately move to
> INIT state see [
> http://www.tcpipguide.com/free/t_DHCPGeneralOperationandClientFiniteStateMachine.htm
> ]
>
>
This implementation of DHCP is not a full fledged implementation, but
rather to support common openstack usecases.
Please see the commit message
of patch 2 - https://patchwork.ozlabs.org/patch/622522/
> > +
> > +#define DHCP_OPT_MSG_TYPE 53
> > +#define DHCP_OPT_END 255
> > +
> > #endif /* dhcp.h */
> > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> > index 96b7c66..0299d49 100644
> > --- a/ovn/controller/lflow.c
> > +++ b/ovn/controller/lflow.c
> > @@ -24,6 +24,7 @@
> > #include "ovn-controller.h"
> > #include "ovn/lib/actions.h"
> > #include "ovn/lib/expr.h"
> > +#include "ovn/lib/ovn-dhcp.h"
> > #include "ovn/lib/ovn-sb-idl.h"
> > #include "packets.h"
> > #include "simap.h"
> > @@ -203,6 +204,13 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> > {
> > uint32_t conj_id_ofs = 1;
> >
> > + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > + const struct sbrec_dhcp_options *dhcp_opt_row;
> > + SBREC_DHCP_OPTIONS_FOR_EACH(dhcp_opt_row, ctx->ovnsb_idl) {
> > + dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> > + dhcp_opt_row->type);
> > + }
> > +
> > const struct sbrec_logical_flow *lflow;
> > SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
> > /* Determine translation of logical table IDs to physical table
> IDs. */
> > @@ -277,6 +285,7 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> > };
> > struct action_params ap = {
> > .symtab = &symtab,
> > + .dhcp_opts = &dhcp_opts,
> > .lookup_port = lookup_port_cb,
> > .aux = &aux,
> > .ct_zones = ct_zones,
> > @@ -360,6 +369,8 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
> > ofpbuf_uninit(&ofpacts);
> > conj_id_ofs += n_conjs;
> > }
> > +
> > + dhcp_opts_destroy(&dhcp_opts);
> > }
> >
> > static void
> > diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> > index 289a995..489ca21 100644
> > --- a/ovn/controller/pinctrl.c
> > +++ b/ovn/controller/pinctrl.c
> > @@ -17,7 +17,9 @@
> >
> > #include "pinctrl.h"
> >
> > +
> > #include "coverage.h"
> > +#include "csum.h"
> > #include "dirs.h"
> > #include "dp-packet.h"
> > #include "flow.h"
> > @@ -27,6 +29,8 @@
> > #include "openvswitch/ofp-print.h"
> > #include "openvswitch/ofp-util.h"
> > #include "openvswitch/vlog.h"
> > +
> > +#include "lib/dhcp.h"
> > #include "ovn-controller.h"
> > #include "ovn/lib/actions.h"
> > #include "ovn/lib/logical-fields.h"
> > @@ -192,13 +196,106 @@ exit:
> > }
> >
> > static void
> > +pinctl_handle_dhcp_offer(struct dp_packet *pkt, struct
> ofputil_packet_in *pin,
> > + struct ofpbuf *userdata, struct ofpbuf
> *continuation)
> > +{
> > + enum ofp_version version = rconn_get_version(swconn);
> > + enum ofputil_protocol proto =
> ofputil_protocol_from_ofp_version(version);
> > +
> > + ovs_be32 *offer_ip = ofpbuf_pull(userdata, sizeof *offer_ip);
> > + const uint8_t *payload = dp_packet_get_udp_payload(pkt);
> > + payload = (payload + sizeof (struct dhcp_header) +
> sizeof(uint32_t));
> > +
> > + if (payload[0] != DHCP_OPT_MSG_TYPE && (payload[2] !=
> DHCP_MSG_DISCOVER ||
> > + payload[2] !=
> DHCP_MSG_REQUEST)) {
> > + /* If its not a valid dhcp packet resume the packet without any
> > + * changes */
> > + goto exit;
> > + }
> > +
> > + uint8_t msg_type;
> > + if (payload[2] == DHCP_MSG_DISCOVER) {
> > + msg_type = DHCP_MSG_OFFER;
> > + } else {
> > + msg_type = DHCP_MSG_ACK;
> > + }
> > +
> > + /* Total dhcp options length will be options stored in the userdata
> +
> > + * 16 bytes.
> > + *
> > + * --------------------------------------------------------------
> > + *| 4 Bytes (dhcp cookie) | 3 Bytes (option type) | dhcp options |
> > + * --------------------------------------------------------------
> > + *| 4 Bytes padding | 1 Byte (option end 0xFF ) | 4 Bytes padding|
> > + * --------------------------------------------------------------
> > + */
> > + uint16_t new_udp_len = UDP_HEADER_LEN + DHCP_HEADER_LEN + \
> > + userdata->size + 16;
> > + size_t new_packet_len = ETH_HEADER_LEN + IP_HEADER_LEN +
> new_udp_len;
> > +
> > + struct dp_packet packet;
> > + dp_packet_init(&packet, new_packet_len);
> > + dp_packet_clear(&packet);
> > + dp_packet_prealloc_tailroom(&packet, new_packet_len);
> > +
> > + dp_packet_put(&packet, dp_packet_pull(pkt, ETH_HEADER_LEN),
> > + ETH_HEADER_LEN);
> > + struct ip_header *ip = dp_packet_put(
> > + &packet, dp_packet_pull(pkt, IP_HEADER_LEN), IP_HEADER_LEN);
> > +
> > + struct udp_header *udp = dp_packet_put(
> > + &packet, dp_packet_pull(pkt, UDP_HEADER_LEN), UDP_HEADER_LEN);
> > +
> > + struct dhcp_header *dhcp_data = dp_packet_put(
> > + &packet, dp_packet_pull(pkt, DHCP_HEADER_LEN), DHCP_HEADER_LEN);
> > + dhcp_data->op = DHCP_OP_REPLY;
> > + dhcp_data->yiaddr = *offer_ip;
> > +
> > + ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> > + dp_packet_put(&packet, &magic_cookie, sizeof(ovs_be32));
> > +
> > + uint8_t *dhcp_options = dp_packet_put_zeros(&packet, userdata->size
> + 12);
> > +
> > + /* Dhcp option - type */
> > + dhcp_options[0] = (uint8_t) DHCP_OPT_MSG_TYPE;
> > + dhcp_options[1] = (uint8_t) 1;
> > + dhcp_options[2] = msg_type;
> > + dhcp_options += 3;
> > +
> > + memcpy(dhcp_options, userdata->data, userdata->size);
> > + dhcp_options += userdata->size;
> > +
> > + /* Padding */
> > + dhcp_options += 4;
> > +
> > + /* End */
> > + dhcp_options[0] = DHCP_OPT_END;
> > +
> > + udp->udp_len = htons(new_udp_len);
> > + ip->ip_tos = 0;
> > + ip->ip_tot_len = htons(IP_HEADER_LEN + new_udp_len);
> > + udp->udp_csum = 0;
> > + ip->ip_csum = 0;
> > + ip->ip_csum = csum(ip, sizeof *ip);
> > +
> > + pin->packet = dp_packet_data(&packet);
> > + pin->packet_len = dp_packet_size(&packet);
> > +
> > +exit:
> > + queue_msg(ofputil_encode_resume(pin, continuation, proto));
> > + dp_packet_uninit(&packet);
> > +}
> > +
> > +static void
> > process_packet_in(const struct ofp_header *msg)
> > {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >
> > struct ofputil_packet_in pin;
> > + struct ofpbuf continuation;
> > enum ofperr error = ofputil_decode_packet_in(msg, true, &pin,
> > - NULL, NULL, NULL);
> > + NULL, NULL,
> &continuation);
> > +
> > if (error) {
> > VLOG_WARN_RL(&rl, "error decoding packet-in: %s",
> > ofperr_to_string(error));
> > @@ -230,6 +327,9 @@ process_packet_in(const struct ofp_header *msg)
> > pinctrl_handle_put_arp(&pin.flow_metadata.flow, &headers);
> > break;
> >
> > + case ACTION_OPCODE_DHCP_OFFER:
> > + pinctl_handle_dhcp_offer(&packet, &pin, &userdata,
> &continuation);
> > + break;
> > default:
> > VLOG_WARN_RL(&rl, "unrecognized packet-in opcode %"PRIu32,
> > ntohl(ah->opcode));
> > @@ -240,6 +340,7 @@ process_packet_in(const struct ofp_header *msg)
> > static void
> > pinctrl_recv(const struct ofp_header *oh, enum ofptype type)
> > {
> > +
> > if (type == OFPTYPE_ECHO_REQUEST) {
> > queue_msg(make_echo_reply(oh));
> > } else if (type == OFPTYPE_GET_CONFIG_REPLY) {
> > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
> > index 5f0bf19..6081cae 100644
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -20,14 +20,18 @@
> > #include "actions.h"
> > #include "byte-order.h"
> > #include "compiler.h"
> > +#include "ovn-dhcp.h"
> > #include "expr.h"
> > #include "lex.h"
> > #include "logical-fields.h"
> > #include "openvswitch/dynamic-string.h"
> > #include "openvswitch/ofp-actions.h"
> > #include "openvswitch/ofpbuf.h"
> > +#include "packets.h"
> > +#include "shash.h"
> > #include "simap.h"
> >
> > +
> > /* Context maintained during actions_parse(). */
> > struct action_context {
> > const struct action_params *ap; /* Parameters. */
> > @@ -186,13 +190,15 @@ add_prerequisite(struct action_context *ctx, const
> char *prerequisite)
> > }
> >
> > static size_t
> > -start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode)
> > +start_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode,
> > + bool pause)
> > {
> > size_t ofs = ofpacts->size;
> >
> > struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts);
> > oc->max_len = UINT16_MAX;
> > oc->reason = OFPR_ACTION;
> > + oc->pause = pause;
> >
> > struct action_header ah = { .opcode = htonl(opcode) };
> > ofpbuf_put(ofpacts, &ah, sizeof ah);
> > @@ -212,7 +218,7 @@ finish_controller_op(struct ofpbuf *ofpacts, size_t
> ofs)
> > static void
> > put_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode)
> > {
> > - size_t ofs = start_controller_op(ofpacts, opcode);
> > + size_t ofs = start_controller_op(ofpacts, opcode, false);
> > finish_controller_op(ofpacts, ofs);
> > }
> >
> > @@ -246,7 +252,9 @@ parse_arp_action(struct action_context *ctx)
> > * converted to OpenFlow, as its userdata. ovn-controller will
> convert the
> > * packet to an ARP and then send the packet and actions back to
> the switch
> > * inside an OFPT_PACKET_OUT message. */
> > - size_t oc_offset = start_controller_op(ctx->ofpacts,
> ACTION_OPCODE_ARP);
> > + /* controller. */
> > + size_t oc_offset = start_controller_op(ctx->ofpacts,
> ACTION_OPCODE_ARP,
> > + false);
> > ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
> > ctx->ofpacts, OFP13_VERSION);
> > finish_controller_op(ctx->ofpacts, oc_offset);
> > @@ -261,6 +269,141 @@ parse_arp_action(struct action_context *ctx)
> > }
> >
> > static bool
> > +parse_dhcp_option(struct action_context *ctx, struct ofpbuf *dhcp_opts)
> > +{
> > + if (ctx->lexer->token.type != LEX_T_ID) {
> > + action_syntax_error(ctx, NULL);
> > + return false;
> > + }
> > +
> > + enum lex_type lookahead = lexer_lookahead(ctx->lexer);
> > + if (lookahead != LEX_T_EQUALS) {
> > + action_syntax_error(ctx, NULL);
> > + return false;
> > + }
> > +
> > + const struct dhcp_opts_map *dhcp_opt = dhcp_opts_find(
> > + ctx->ap->dhcp_opts, ctx->lexer->token.s);
> > +
> > + if (!dhcp_opt) {
> > + action_syntax_error(ctx, "expecting valid dhcp option.");
> > + return false;
> > + }
> > +
> > + lexer_get(ctx->lexer);
> > + lexer_get(ctx->lexer);
> > +
> > + struct expr_constant_set cs;
> > + memset(&cs, 0, sizeof(struct expr_constant_set));
> > + if (!expr_parse_constant_set(ctx->lexer, NULL, &cs)) {
> > + action_syntax_error(ctx, "invalid dhcp option values");
> > + return false;
> > + }
> > +
> > + if (!lexer_match(ctx->lexer, LEX_T_COMMA) && (
> > + ctx->lexer->token.type != LEX_T_RPAREN)) {
> > + action_syntax_error(ctx, NULL);
> > + return false;
> > + }
> > +
> > +
> > + if (dhcp_opt->code == 0) {
> > + /* offer-ip */
> > + ofpbuf_push(dhcp_opts, &cs.values[0].value.ipv4,
> sizeof(ovs_be32));
> > + goto exit;
> > + }
> > +
> > + uint8_t *opt_header = ofpbuf_put_uninit(dhcp_opts, 2);
> > + opt_header[0] = dhcp_opt->code;
> > +
> > +
> > + switch(dhcp_opt->type) {
> > + case DHCP_OPT_TYPE_BOOL:
> > + case DHCP_OPT_TYPE_UINT8:
> > + opt_header[1] = 1;
> > + uint8_t val = cs.values[0].value.u8[127];
> > + ofpbuf_put(dhcp_opts, &val, 1);
> > + break;
> > +
> > + case DHCP_OPT_TYPE_UINT16:
> > + opt_header[1] = 2;
> > + ofpbuf_put(dhcp_opts, &cs.values[0].value.be16[63], 2);
> > + break;
> > +
> > + case DHCP_OPT_TYPE_UINT32:
> > + opt_header[1] = 4;
> > + ofpbuf_put(dhcp_opts, &cs.values[0].value.be32[31], 4);
> > + break;
> > +
> > + case DHCP_OPT_TYPE_IP4:
> > + opt_header[1] = 0;
> > + for (size_t i = 0; i < cs.n_values; i++) {
> > + ofpbuf_put(dhcp_opts, &cs.values[i].value.ipv4,
> sizeof(ovs_be32));
> > + opt_header[1] += sizeof(ovs_be32);
> > + }
> > + break;
> > +
> > + case DHCP_OPT_TYPE_STATIC_ROUTES:
> > + {
> > + size_t no_of_routes = cs.n_values;
> > + if (no_of_routes % 2) {
> > + no_of_routes -= 1;
> > + }
> > + opt_header[1] = 0;
> > + for (size_t i = 0; i < no_of_routes; i+= 2) {
> > + uint8_t plen = 32;
> > + if (cs.values[i].masked) {
> > + plen = (uint8_t)
> ip_count_cidr_bits(cs.values[i].mask.ipv4);
> > + }
> > + ofpbuf_put(dhcp_opts, &plen, 1);
> > + ofpbuf_put(dhcp_opts, &cs.values[i].value.ipv4, plen / 8);
> > + ofpbuf_put(dhcp_opts, &cs.values[i + 1].value.ipv4,
> > + sizeof(ovs_be32));
> > + opt_header[1] += (1 + (plen / 8) + sizeof(ovs_be32)) ;
> > + }
> > + break;
> > + }
> > +
> > + case DHCP_OPT_TYPE_STR:
> > + opt_header[1] = strlen(cs.values[0].string);
> > + ofpbuf_put(dhcp_opts, cs.values[0].string, opt_header[1]);
> > + break;
> > + }
> > +
> > +exit:
> > + expr_constant_set_destroy(&cs);
> > + return true;
> > +}
> > +
> > +static void
> > +parse_dhcp_offer_action(struct action_context *ctx)
> > +{
> > + if (!lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> > + action_syntax_error(ctx, "expecting `('");
> > + return;
> > + }
> > +
> > + uint64_t dhcp_opts_stub[1024 / 8];
> > + struct ofpbuf dhcp_opts = OFPBUF_STUB_INITIALIZER(dhcp_opts_stub);
> > +
> > + /* Parse inner actions. */
> > + while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> > + if (!parse_dhcp_option(ctx, &dhcp_opts)) {
> > + return;
> > + }
> > + }
> > +
> > + /* controller. */
> > + size_t oc_offset = start_controller_op(ctx->ofpacts,
> > + ACTION_OPCODE_DHCP_OFFER,
> true);
> > + ofpbuf_put(ctx->ofpacts, dhcp_opts.data, dhcp_opts.size);
> > + finish_controller_op(ctx->ofpacts, oc_offset);
> > +
> > + /* Free memory. */
> > + ofpbuf_uninit(&dhcp_opts);
> > +}
> > +
> > +static bool
> > action_force_match(struct action_context *ctx, enum lex_type t)
> > {
> > if (lexer_match(ctx->lexer, t)) {
> > @@ -475,6 +618,8 @@ parse_action(struct action_context *ctx)
> > parse_get_arp_action(ctx);
> > } else if (lexer_match_id(ctx->lexer, "put_arp")) {
> > parse_put_arp_action(ctx);
> > + } else if (lexer_match_id(ctx->lexer, "dhcp_offer")) {
> > + parse_dhcp_offer_action(ctx);
> > } else {
> > action_syntax_error(ctx, "expecting action");
> > }
> > diff --git a/ovn/lib/actions.h b/ovn/lib/actions.h
> > index 29af06f..6c74878 100644
> > --- a/ovn/lib/actions.h
> > +++ b/ovn/lib/actions.h
> > @@ -44,6 +44,11 @@ enum action_opcode {
> > * MFF_ETH_SRC = mac
> > */
> > ACTION_OPCODE_PUT_ARP,
> > +
> > + /* "dhcp_offer(...dhcp actions ...}".
> > + *
> > + */
> > + ACTION_OPCODE_DHCP_OFFER,
> > };
> >
> > /* Header. */
> > @@ -58,6 +63,9 @@ struct action_params {
> > * expr_parse()). */
> > const struct shash *symtab;
> >
> > + /* hmap of 'struct dhcp_opts_map' to support 'dhcp_offer' */
> > + const struct hmap *dhcp_opts;
> > +
> > /* Looks up logical port 'port_name'. If found, stores its port
> number in
> > * '*portp' and returns true; otherwise, returns false. */
> > bool (*lookup_port)(const void *aux, const char *port_name,
> > diff --git a/ovn/lib/automake.mk b/ovn/lib/automake.mk
> > index 9e09352..7294742 100644
> > --- a/ovn/lib/automake.mk
> > +++ b/ovn/lib/automake.mk
> > @@ -10,7 +10,8 @@ ovn_lib_libovn_la_SOURCES = \
> > ovn/lib/expr.h \
> > ovn/lib/lex.c \
> > ovn/lib/lex.h \
> > - ovn/lib/logical-fields.h
> > + ovn/lib/logical-fields.h \
> > + ovn/lib/ovn-dhcp.h
> > nodist_ovn_lib_libovn_la_SOURCES = \
> > ovn/lib/ovn-nb-idl.c \
> > ovn/lib/ovn-nb-idl.h \
> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > index f274ab4..05a5c8e 100644
> > --- a/ovn/lib/expr.c
> > +++ b/ovn/lib/expr.c
> > @@ -405,39 +405,6 @@ expr_print(const struct expr *e)
> >
> > /* Parsing. */
> >
> > -/* Type of a "union expr_constant" or "struct expr_constant_set". */
> > -enum expr_constant_type {
> > - EXPR_C_INTEGER,
> > - EXPR_C_STRING
> > -};
> > -
> > -/* A string or integer constant (one must know which from context). */
> > -union expr_constant {
> > - /* Integer constant.
> > - *
> > - * The width of a constant isn't always clear, e.g. if you write
> "1",
> > - * there's no way to tell whether you mean for that to be a 1-bit
> constant
> > - * or a 128-bit constant or somewhere in between. */
> > - struct {
> > - union mf_subvalue value;
> > - union mf_subvalue mask; /* Only initialized if 'masked'. */
> > - bool masked;
> > -
> > - enum lex_format format; /* From the constant's lex_token. */
> > - };
> > -
> > - /* Null-terminated string constant. */
> > - char *string;
> > -};
> > -
> > -/* A collection of "union expr_constant"s of the same type. */
> > -struct expr_constant_set {
> > - union expr_constant *values; /* Constants. */
> > - size_t n_values; /* Number of constants. */
> > - enum expr_constant_type type; /* Type of the constants. */
> > - bool in_curlies; /* Whether the constants were in {}.
> */
> > -};
> > -
> > /* A reference to a symbol or a subfield of a symbol.
> > *
> > * For string fields, ofs and n_bits are 0. */
> > @@ -457,7 +424,6 @@ struct expr_context {
> >
> > struct expr *expr_parse__(struct expr_context *);
> > static void expr_not(struct expr *);
> > -static void expr_constant_set_destroy(struct expr_constant_set *);
> > static bool parse_field(struct expr_context *, struct expr_field *);
> >
> > static bool
> > @@ -838,7 +804,7 @@ parse_constant_set(struct expr_context *ctx, struct
> expr_constant_set *cs)
> > return ok;
> > }
> >
> > -static void
> > +void
> > expr_constant_set_destroy(struct expr_constant_set *cs)
> > {
> > if (cs) {
> > @@ -2928,3 +2894,15 @@ exit:
> > }
> > return ctx.error;
> > }
> > +
> > +bool
> > +expr_parse_constant_set(struct lexer *lexer, const struct shash *symtab,
> > + struct expr_constant_set *cs)
> > +{
> > + struct expr_context ctx = {
> > + .lexer = lexer,
> > + .symtab = symtab,
> > + };
> > +
> > + return parse_constant_set(&ctx, cs);
> > +}
> > diff --git a/ovn/lib/expr.h b/ovn/lib/expr.h
> > index 1327789..99e7aaf 100644
> > --- a/ovn/lib/expr.h
> > +++ b/ovn/lib/expr.h
> > @@ -391,4 +391,41 @@ char *expr_parse_field(struct lexer *, int n_bits,
> bool rw,
> > const struct shash *symtab, struct mf_subfield *,
> > struct expr **prereqsp);
> >
> > +/* Type of a "union expr_constant" or "struct expr_constant_set". */
> > +enum expr_constant_type {
> > + EXPR_C_INTEGER,
> > + EXPR_C_STRING
> > +};
> > +
> > +/* A string or integer constant (one must know which from context). */
> > +union expr_constant {
> > + /* Integer constant.
> > + *
> > + * The width of a constant isn't always clear, e.g. if you write
> "1",
> > + * there's no way to tell whether you mean for that to be a 1-bit
> constant
> > + * or a 128-bit constant or somewhere in between. */
> > + struct {
> > + union mf_subvalue value;
> > + union mf_subvalue mask; /* Only initialized if 'masked'. */
> > + bool masked;
> > +
> > + enum lex_format format; /* From the constant's lex_token. */
> > + };
> > +
> > + /* Null-terminated string constant. */
> > + char *string;
> > +};
> > +
> > +/* A collection of "union expr_constant"s of the same type. */
> > +struct expr_constant_set {
> > + union expr_constant *values; /* Constants. */
> > + size_t n_values; /* Number of constants. */
> > + enum expr_constant_type type; /* Type of the constants. */
> > + bool in_curlies; /* Whether the constants were in {}.
> */
> > +};
> > +
> > +bool expr_parse_constant_set(struct lexer *, const struct shash *symtab,
> > + struct expr_constant_set *cs);
> > +void expr_constant_set_destroy(struct expr_constant_set *cs);
> > +
> > #endif /* ovn/expr.h */
> > diff --git a/ovn/lib/ovn-dhcp.h b/ovn/lib/ovn-dhcp.h
> > new file mode 100644
> > index 0000000..fae944f
> > --- /dev/null
> > +++ b/ovn/lib/ovn-dhcp.h
> > @@ -0,0 +1,121 @@
> > +/*
> > + * Copyright (c) 2016 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#ifndef OVN_DHCP_H
> > +#define OVN_DHCP_H 1
> > +
> > +#include "hmap.h"
> > +#include "hash.h"
> > +
> > +enum dhcp_opt_type {
> > + DHCP_OPT_TYPE_BOOL,
> > + DHCP_OPT_TYPE_UINT8,
> > + DHCP_OPT_TYPE_UINT16,
> > + DHCP_OPT_TYPE_UINT32,
> > + DHCP_OPT_TYPE_IP4,
> > + DHCP_OPT_TYPE_STATIC_ROUTES,
> > + DHCP_OPT_TYPE_STR
> > +};
> > +
> > +struct dhcp_opts_map {
> > + struct hmap_node hmap_node;
> > + char *name;
> > + size_t code;
> > + size_t type;
> > +};
> > +
> > +#define DHCP_OPTION(NAME, CODE, TYPE) \
> > + {.name = NAME, .code = CODE, .type = TYPE}
> > +
> > +#define OFFERIP DHCP_OPTION("offerip", 0,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_NETMASK DHCP_OPTION("netmask", 1,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_ROUTER DHCP_OPTION("router", 3, DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_DNS_SERVER DHCP_OPTION("dns_server", 6,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_LOG_SERVER DHCP_OPTION("log_server", 7,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_LPR_SERVER DHCP_OPTION("lpr_server", 9,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_SWAP_SERVER DHCP_OPTION("swap_server", 16,
> DHCP_OPT_TYPE_IP4)
> > +
> > +#define DHCP_OPT_POLICY_FILTER \
> > + DHCP_OPTION("policy_filter", 21, DHCP_OPT_TYPE_IP4)
> > +
> > +#define DHCP_OPT_ROUTER_SOLICITATION \
> > + DHCP_OPTION("router_solicitation", 32, DHCP_OPT_TYPE_IP4)
> > +
> > +#define DHCP_OPT_NIS_SERVER DHCP_OPTION("nis_server", 41,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_NTP_SERVER DHCP_OPTION("ntp_server", 42,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_SERVER_ID DHCP_OPTION("server_id", 54,
> DHCP_OPT_TYPE_IP4)
> > +#define DHCP_OPT_TFTP_SERVER DHCP_OPTION("tftp_server", 66,
> DHCP_OPT_TYPE_IP4)
>
> AFAIK DHCP option tftp_server (66) is tricky. Allegedly there are
> clients who use it. However, the KVM PXE implementation asks for this
> option via DHCP Parameters list option, but still unconditionally ends
> up using SIADDR field to locate tftp server with initial PXE boot
> image. If tftp_server happens to be an IP address, then I think you
> may want to populate it in SIADDR field as well.
>
>
> > +
> > +#define DHCP_OPT_CLASSLESS_STATIC_ROUTE \
> > + DHCP_OPTION("classless_static_route", 121,
> DHCP_OPT_TYPE_STATIC_ROUTES)
>
> I think Linux dhclient is using option 121, but microsoft dhcp client
> is using option 249 for classless static routes See
> https://ercpe.de/blog/pushing-static-routes-with-isc-dhcp-server for
> more details.
>
>
The list of supported DHCP options are stored in a new table
"DHCP_Options" in SB DB.
Please see the v5 of the patchset. If we need to support option 249 or any
other option,
we just need to update the 'ovn-northd' to add the new option in the table.
I think we can add new options if there is a requirement. I have no idea if
Windows clients are used or not. If you think this needs to be supported
please let me know, I can spin another patchset.
> I think ideally DHCP server should honor option 54 (the DHCP options
> client understands) and then server should return either 121 or 249.
>
>
The DHCP options to reply is defined by the CMS (cloud management
system). So I think CMS can decide to put option 121 or 249 by adding
these options in the NB DB table "Subnet", "dhcp_options" column.
>
>
> > +
> > +#define DHCP_OPT_IP_FORWARD_ENABLE \
> > + DHCP_OPTION("ip_forward_enable", 19, DHCP_OPT_TYPE_BOOL)
> > +#define DHCP_OPT_ROUTER_DISCOVERY \
> > + DHCP_OPTION("router_discovery", 31, DHCP_OPT_TYPE_BOOL)
> > +#define DHCP_OPT_ETHERNET_ENCAP \
> > + DHCP_OPTION("ethernet_encap", 36, DHCP_OPT_TYPE_BOOL)
> > +
> > +#define DHCP_OPT_DEFAULT_TTL \
> > + DHCP_OPTION("default_ttl", 23, DHCP_OPT_TYPE_UINT8)
> > +
> > +#define DHCP_OPT_TCP_TTL DHCP_OPTION("tcp_ttl", 37,
> DHCP_OPT_TYPE_UINT8)
> > +#define DHCP_OPT_MTU DHCP_OPTION("mtu", 26, DHCP_OPT_TYPE_UINT16)
> > +#define DHCP_OPT_LEASE_TIME \
> > + DHCP_OPTION("lease_time", 51, DHCP_OPT_TYPE_UINT32)
> > +
> > +static inline uint32_t
> > +dhcp_opt_hash(char *opt_name)
> > +{
> > + return hash_string(opt_name, 0);
> > +}
> > +
> > +static inline struct dhcp_opts_map *
> > +dhcp_opts_find(const struct hmap *dhcp_opts, char *opt_name)
> > +{
> > + struct dhcp_opts_map *dhcp_opt;
> > + HMAP_FOR_EACH_WITH_HASH (dhcp_opt, hmap_node,
> dhcp_opt_hash(opt_name),
> > + dhcp_opts) {
> > + if (!strcmp(dhcp_opt->name, opt_name)) {
> > + return dhcp_opt;
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static inline void
> > +dhcp_opt_add(struct hmap *dhcp_opts, char *opt_name, size_t code,
> size_t type)
> > +{
> > + struct dhcp_opts_map *dhcp_opt = xzalloc(sizeof *dhcp_opt);
> > + dhcp_opt->name = xstrdup(opt_name);
> > + dhcp_opt->code = code;
> > + dhcp_opt->type = type;
> > + hmap_insert(dhcp_opts, &dhcp_opt->hmap_node,
> dhcp_opt_hash(opt_name));
> > +}
> > +
> > +static inline void
> > +dhcp_opts_destroy(struct hmap *dhcp_opts)
> > +{
> > + struct dhcp_opts_map *dhcp_opt;
> > + HMAP_FOR_EACH_POP(dhcp_opt, hmap_node, dhcp_opts) {
> > + free(dhcp_opt->name);
> > + free(dhcp_opt);
> > + }
> > + hmap_destroy(dhcp_opts);
> > +}
> > +
> > +#endif /* OVN_DHCP_H */
> > diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema
> > index 06e8a07..1b20d45 100644
> > --- a/ovn/ovn-sb.ovsschema
> > +++ b/ovn/ovn-sb.ovsschema
> > @@ -1,7 +1,7 @@
> > {
> > "name": "OVN_Southbound",
> > - "version": "1.3.0",
> > - "cksum": "654726257 5528",
> > + "version": "1.4.0",
> > + "cksum": "1002497001 5942",
> > "tables": {
> > "Chassis": {
> > "columns": {
> > @@ -110,4 +110,13 @@
> > "ip": {"type": "string"},
> > "mac": {"type": "string"}},
> > "indexes": [["logical_port", "ip"]],
> > + "isRoot": true},
> > + "DHCP_Options": {
> > + "columns": {
> > + "name": {"type": "string"},
> > + "code": {
> > + "type": {"key": {"type": "integer",
> > + "minInteger": 0, "maxInteger":
> 254}}},
> > + "type": {"type": {"key": {"type": "integer",
> > + "minInteger": 0, "maxInteger": 6}}}},
> > "isRoot": true}}}
> > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> > index efd2f9a..d699c2e 100644
> > --- a/ovn/ovn-sb.xml
> > +++ b/ovn/ovn-sb.xml
> > @@ -1022,6 +1022,35 @@
> >
> > <p><b>Example:</b> <code>put_arp(inport, arp.spa,
> arp.sha);</code></p>
> > </dd>
> > +
> > + <dt>
> > + <code>dhcp_offer(<var>D1</var> = <var>V1</var>, <var>D2</var>
> = <var>V2</var>, ...,<var>Dn</var> = <var>Vn</var>);</code>
> > + </dt>
> > +
> > + <dd>
> > + <p>
> > + <b>Parameters</b>: DHCP option name <var>Dn</var>, DHCP
> option
> > + value <var>Vn</var>.
> > + </p>
> > +
> > + <p>
> > + Valid only in the ingress pipeline.
> > + </p>
> > +
> > + <p>
> > + DHCP options and values defined in the parameters are added
> in
> > + the 'DHCP options' field of the incoming DHCP
> DISCOVER/REQUEST
> > + packet and resumed in the pipeline.
> > + </p>
> > +
> > + <p>
> > + <b>Example:</b>
> > + <code>
> > + dhcp_offer(offerip = 10.0.0.2, router = 10.0.0.1,
> > + netmask = 255.255.255.0, dns_server =
> {8.8.8.8,7.7.7.7});
> > + </code>
> > + </p>
> > + </dd>
> > </dl>
> >
> > <p>
> > @@ -1511,4 +1540,19 @@ tcp.flags = RST;
> > The Ethernet address to which the IP is bound.
> > </column>
> > </table>
> > +
> > + <table name="DHCP_Options" title="DHCP Options supported by native
> OVN DHCP">
> > +
> > + <column name="name">
> > + Name of the DHCP option
> > + </column>
> > +
> > + <column name="code">
> > + DHCP option code
> > + </column>
> > +
> > + <column name="type">
> > + Data type of the DHCP option code
> > + </column>
> > + </table>
> > </database>
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index a5c6074..15fcb50 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -324,6 +324,7 @@ tests_ovstest_SOURCES = \
> > tests/test-odp.c \
> > tests/test-ofpbuf.c \
> > tests/test-ovn.c \
> > + tests/test-ovn-dhcp.c \
> > tests/test-packets.c \
> > tests/test-random.c \
> > tests/test-rcu.c \
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index b9d7ada..c439947 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -2545,3 +2545,42 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> >
> > AT_CLEANUP
> >
> > +AT_SETUP([ovn -- dhcp_offer action])
> >
> +dhcp_options="offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400"
> > +echo "dhcp_options = $dhcp_options"
> > +
> > +expected_dhcp_opts="0a00000403040a0000010104fffffe001a020578"
> > +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> > + [0], [ignore])
> > +
> >
> +dhcp_options="offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.255.0,mtu=1400,\
> > +ip_forward_enable=1,default_ttl=121,dns_server={8.8.8.8,7.7.7.7},\
> > +classless_static_route={30.0.0.0/24,10.0.0.4,40.0.0.0/16,10.0.0.6,\
> > +0.0.0.0/0,10.0.0.1},ethernet_encap=1"
> > +
> > +# offerip=10.0.0.4 --> 0a000004
> > +# router=10.0.0.1 --> 03040a000001
> > +# netmask=255.255.255.0 --> 0104ffffff00
> > +# mtu=1400 --> 1a020578
> > +# ip_forward_enable-1 --> 130101
> > +# default_ttl=121 --> 170179
> > +# dns_server={8.8.8.8,7.7.7.7} --> 06080808080807070707
> > +# classless_static_route= --> 7914
> > +# {30.0.0.0/24,10.0.0.4 --> 181e00000a000004
> > +# 40.0.0.0/16,10.0.0.6 --> 1028000a000006
> > +# 0.0.0.0/0,10.0.0.1} --> 000a000001
> > +# ethernet_encap=1 --> 240101
> > +# router_discovery=0 --> 1f0100
> > +
> >
> +expected_dhcp_opts="0a00000403040a0000010104ffffff001a020578130101170179\
> > +060808080808070707077914181e00000a0000041028000a000006000a000001240101"
> > +
> > +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> > + [0], [ignore])
> > +
> > +dhcp_options="offerip=10.0.0.4,invalid_options=inv_value"
> > +expected_dhcp_opts="00"
> > +AT_CHECK([ovstest test-ovn-dhcp-offer-action $dhcp_options
> $expected_dhcp_opts],
> > + [1], [ignore])
> > +
> > +AT_CLEANUP
> > diff --git a/tests/test-ovn-dhcp.c b/tests/test-ovn-dhcp.c
> > new file mode 100644
> > index 0000000..60726d9
> > --- /dev/null
> > +++ b/tests/test-ovn-dhcp.c
> > @@ -0,0 +1,117 @@
> > +/* Copyright (c) 2016 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include <assert.h>
> > +#include <config.h>
> > +#include "command-line.h"
> > +#include "openvswitch/ofp-actions.h"
> > +#include "ovstest.h"
> > +#include "ovn/lib/actions.h"
> > +#include "ovn/lib/ovn-dhcp.h"
> > +
> > +
> > +static void
> > +test_dhcp_offer_action(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> > +{
> > + if (argc != 3) {
> > + printf("Usage: %s dhcp-options expected-dhcp-opt-codes",
> argv[0]);
> > + exit(1);
> > + }
> > +
> > + struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> > +
> > + dhcp_opt_add(&dhcp_opts, "offerip", 0, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "netmask", 1, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "router", 3, DHCP_OPT_TYPE_IP4);
>
> DHCP option 3 (Routers) is a list of IP address a (See setion 3.5 of
> RFC2132), but DHCP options 1 (subnet mask) is a single IP address (see
> 3.3 of RFC2132). However, here you are using the same type for all of
> these options - DHCP_OPT_TYPE_IP4. Shouldn't you break down
> DHCP_OPT_TYPE_IP4 option into two types - one that represents a single
> IP address and another that represents a list of IP addresses?
>
>
> I think it should be fine. CMS has to define it properly.
If CMS want to include a list of IP addresses for example "dns_nameserver",
it
should define as "dns_nameserver={7.7.7.7, 8.8.8.8}". If it wants to
include
only one ip, it should define as "dns_nameserver=7.7.7.7".
This has been documented in the next patch which adds logical flows in
ovn-northd.
> > + dhcp_opt_add(&dhcp_opts, "dns_server", 6, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "log_server", 7, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "lpr_server", 9, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "swap_server", 16, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "policy_filter", 21, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "router_solicitation", 32,
> DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "nis_server", 41, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "ntp_server", 42, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "server_id", 54, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "tftp_server", 66, DHCP_OPT_TYPE_IP4);
> > + dhcp_opt_add(&dhcp_opts, "classless_static_route", 121,
> > + DHCP_OPT_TYPE_STATIC_ROUTES);
> > + dhcp_opt_add(&dhcp_opts, "ip_forward_enable", 19,
> DHCP_OPT_TYPE_BOOL);
> > + dhcp_opt_add(&dhcp_opts, "router_discovery", 31,
> DHCP_OPT_TYPE_BOOL);
> > + dhcp_opt_add(&dhcp_opts, "ethernet_encap", 36, DHCP_OPT_TYPE_BOOL);
> > + dhcp_opt_add(&dhcp_opts, "default_ttl", 23, DHCP_OPT_TYPE_UINT8);
> > + dhcp_opt_add(&dhcp_opts, "tcp_ttl", 37, DHCP_OPT_TYPE_UINT8);
> > + dhcp_opt_add(&dhcp_opts, "mtu", 26, DHCP_OPT_TYPE_UINT16);
> > + dhcp_opt_add(&dhcp_opts, "lease_time", 51, DHCP_OPT_TYPE_UINT32);
>
> Should option 58 (renewal time) and 59 (rebinding time) be in this list as
> well?
>
> If they are absent, then it seems that dhclient3 implementation
> derives renewal time and rebinding time from lease_time option by
> setting both of them to a "randomized" value that is lower so that all
> DHCP clients would not try to renew their leases at the same time. I
> don't know what other DHCP client implementations do in such scenarios
> where 58 and 59 are absent. But the worst case scenario is that out
> there could be some implementations that set option 58, 59 to the
> same value as 51 and then there is a chance that client may unassign
> IP address from interface and proceed with renewal.
>
>
This can also be easily supported by adding these options in the
"DHCP_Options" SB DB table.
Also CMS should add these options in the "Subnet" NB DB table.
Please let me know if this needs to be supported in this patchset so that I
can spin another one.
> IIRC then ISC dhcpd server always returns all three options (e.g. 58,
> 59, 51) to client. However, server is the one generating 58 and 59 to
> randomized lower value.
>
>
>
>
>
>
> > +
> > + struct action_params ap = {
> > + .dhcp_opts = &dhcp_opts,
> > + };
> > +
> > +
> > + char *actions = xasprintf("dhcp_offer(%s);", argv[1]);
> > + uint64_t ofpacts_stub[128 / 8];
> > + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(
> > + ofpacts_stub);
> > + struct expr *prereqs;
> > + char *error;
> > +
> > + error = actions_parse_string(actions, &ap, &ofpacts, &prereqs);
> > + dhcp_opts_destroy(&dhcp_opts);
> > + free(actions);
> > + if (error) {
> > + printf("actions_parse_string failed with error - %s\n", error);
> > + free(error);
> > + exit(1);
> > + }
> > +
> > + if (ofpacts.size < (sizeof(struct ofpact_controller) +
> > + sizeof(struct action_header))) {
> > + ovs_fatal(1, "Error. dhcp_offer parse action failed : "
> > + " ofpact_controller not configured");
> > + }
> > +
> > + struct ofpact_controller *oc = ofpbuf_pull(&ofpacts, sizeof *oc);
> > + if (!oc->pause) {
> > + ovs_fatal(1, "Error. dhcp_offer parse action failed : pause
> flag "
> > + " not set in ofpact_controller");
> > + }
> > + struct action_header *ah = ofpbuf_pull(&ofpacts, sizeof *ah);
> > + if (ah->opcode != htonl(ACTION_OPCODE_DHCP_OFFER)) {
> > + ovs_fatal(1, "Error. dhcp_offer parse action failed :
> dhcp_offer "
> > + "action header flag not set");
> > + }
> > +
> > + uint64_t expected_dhcp_opts_stub[128 / 8];
> > + struct ofpbuf expected_dhcp_opts = OFPBUF_STUB_INITIALIZER(
> > + expected_dhcp_opts_stub);
> > + if (ofpbuf_put_hex(&expected_dhcp_opts, argv[2], NULL)[0] != '\0') {
> > + ovs_fatal(1, "Error. Invalid expected dhcp opts");
> > + }
> > +
> > + 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",
> > + expected_dhcp_opts.size + sizeof *ah,
> oc->userdata_len);
> > + }
> > +
> > + if (memcmp(ofpacts.data, expected_dhcp_opts.data,
> expected_dhcp_opts.size)) {
> > + ovs_fatal(1, "Error. dhcp_offer parse action failed : dhcp opts
> are"
> > + " not as expected");
> > + }
> > +
> > + exit(0);
> > +}
> > +
> > +OVSTEST_REGISTER("test-ovn-dhcp-offer-action", test_dhcp_offer_action);
> > --
> > 2.5.5
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
