With clarifications requested below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> lib/ofp-prop.c | 98 +++++++++++++++++++++++++++++++++++------
> lib/ofp-prop.h | 57 ++++++++++++++++++++----
> lib/ofp-util.c | 134 ++++++++++++---------------------------------------------
> 3 files changed, 160 insertions(+), 129 deletions(-)
>
> diff --git a/lib/ofp-prop.c b/lib/ofp-prop.c
> index 2d90e1b..83d72ab 100644
> --- a/lib/ofp-prop.c
> +++ b/lib/ofp-prop.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -21,8 +21,21 @@
> #include "byte-order.h"
> #include "ofpbuf.h"
> #include "ofp-errors.h"
> +#include "openvswitch/vlog.h"
> #include "util.h"
>
> +static uint32_t
> +ofpprop_type_to_experimenter(uint64_t type)
> +{
> + return type >> 32;
> +}
There are some ambiguities here. What is here termed ‘experimenter’ is
elsewhere also called ‘experimenter ID’, and what is below termed ‘exp_id’ is
elsewhere also called 'experimenter type’ or ‘ex_type’.
> +
> +static uint32_t
> +ofpprop_type_to_exp_id(uint64_t type)
> +{
> + return type & UINT32_MAX;
> +}
> +
> /* Pulls a property, beginning with struct ofp_prop_header, from the beginning
> * of 'msg'. Stores the type of the property in '*typep' and, if 'property'
> is
> * nonnull, the entire property, including the header, in '*property'.
> Returns
> @@ -33,7 +46,8 @@
> * you can use ofpprop_pull() for that case. */
> enum ofperr
> ofpprop_pull__(struct ofpbuf *msg, struct ofpbuf *property,
> - unsigned int alignment, uint16_t *typep)
> + unsigned int alignment, unsigned int min_exp,
It would be nice to justify the existence of ‘min_exp’ in the comment above.
> + uint64_t *typep)
> {
> struct ofp_prop_header *oph;
> unsigned int padded_len;
> @@ -50,9 +64,31 @@ ofpprop_pull__(struct ofpbuf *msg, struct ofpbuf *property,
> return OFPERR_OFPBPC_BAD_LEN;
> }
>
> - *typep = ntohs(oph->type);
> + uint16_t type = ntohs(oph->type);
> + if (type < min_exp) {
> + *typep = type;
> + } else {
> + struct ofp_prop_experimenter *ope = msg->data;
> + if (len < sizeof *ope) {
> + return OFPERR_OFPBPC_BAD_LEN;
> + }
> +
> + if (!ope->experimenter) {
> + /* Reject experimenter 0 because it yields ambiguity with
> standard
> + * property types. */
> + return OFPERR_OFPBPC_BAD_EXPERIMENTER;
> + }
> +
> + *typep = OFPPROP_EXP(ntohl(ope->experimenter), ntohl(ope->exp_type));
> + }
> +
> if (property) {
> ofpbuf_use_const(property, msg->data, len);
> + msg->header = msg->data;
> + msg->msg = ((uint8_t *) msg->data
> + + (type < min_exp
> + ? sizeof(struct ofp_prop_header)
> + : sizeof(struct ofp_prop_experimenter)));
> }
> ofpbuf_pull(msg, padded_len);
> return 0;
> @@ -66,15 +102,15 @@ ofpprop_pull__(struct ofpbuf *msg, struct ofpbuf
> *property,
> * This function pulls the property's stated size padded out to a multiple of
> * 8 bytes, which is the common case for OpenFlow properties. */
> enum ofperr
> -ofpprop_pull(struct ofpbuf *msg, struct ofpbuf *property, uint16_t *typep)
> +ofpprop_pull(struct ofpbuf *msg, struct ofpbuf *property, uint64_t *typep)
> {
> - return ofpprop_pull__(msg, property, 8, typep);
> + return ofpprop_pull__(msg, property, 8, 0xffff, typep);
It would be nice to document the use of '0xffff’ here.
> }
>
> /* Adds a property with the given 'type' and 'len'-byte contents 'value' to
> * 'msg', padding the property out to a multiple of 8 bytes. */
> void
> -ofpprop_put(struct ofpbuf *msg, uint16_t type, const void *value, size_t len)
> +ofpprop_put(struct ofpbuf *msg, uint64_t type, const void *value, size_t len)
> {
> size_t start_ofs = ofpprop_start(msg, type);
> ofpbuf_put(msg, value, len);
> @@ -84,7 +120,7 @@ ofpprop_put(struct ofpbuf *msg, uint16_t type, const void
> *value, size_t len)
> /* Appends a property to 'msg' whose type is 'type' and whose contents is a
> * series of property headers, one for each 1-bit in 'bitmap'. */
> void
> -ofpprop_put_bitmap(struct ofpbuf *msg, uint16_t type, uint64_t bitmap)
> +ofpprop_put_bitmap(struct ofpbuf *msg, uint64_t type, uint64_t bitmap)
> {
> size_t start_ofs = ofpprop_start(msg, type);
> for (; bitmap; bitmap = zero_rightmost_1bit(bitmap)) {
> @@ -98,14 +134,21 @@ ofpprop_put_bitmap(struct ofpbuf *msg, uint16_t type,
> uint64_t bitmap)
> * ofpprop_end(). Returns the offset of the beginning of the property (to
> pass
> * to ofpprop_end() later). */
> size_t
> -ofpprop_start(struct ofpbuf *msg, uint16_t type)
> +ofpprop_start(struct ofpbuf *msg, uint64_t type)
> {
> size_t start_ofs = msg->size;
> - struct ofp_prop_header *oph;
> -
> - oph = ofpbuf_put_uninit(msg, sizeof *oph);
> - oph->type = htons(type);
> - oph->len = htons(4); /* May be updated later by ofpprop_end(). */
> + if (!ofpprop_is_experimenter(type)) {
> + struct ofp_prop_header *oph = ofpbuf_put_uninit(msg, sizeof *oph);
> + oph->type = htons(type);
> + oph->len = htons(4);
> + } else {
> + struct ofp_prop_experimenter *ope
> + = ofpbuf_put_uninit(msg, sizeof *ope);
> + ope->type = htons(0xffff);
> + ope->len = htons(12);
> + ope->experimenter = htonl(ofpprop_type_to_experimenter(type));
> + ope->exp_type = htonl(ofpprop_type_to_exp_id(type));
> + }
> return start_ofs;
> }
>
> @@ -123,3 +166,32 @@ ofpprop_end(struct ofpbuf *msg, size_t start_ofs)
> ofpbuf_padto(msg, ROUND_UP(msg->size, 8));
> }
>
> +enum ofperr
> +ofpprop_unknown(struct vlog_module *module, bool loose, const char *msg,
> + uint64_t type)
> +{
> + bool is_experimenter = ofpprop_is_experimenter(type);
> +
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> + enum vlog_level level = loose ? VLL_DBG : VLL_WARN;
> + if (!is_experimenter) {
> + vlog_rate_limit(module, level, &rl, "unknown %s property type
> %"PRId64,
> + msg, type);
> + } else {
> + vlog_rate_limit(module, level, &rl,
> + "unknown %s property type for experimenter "
> + "0x%"PRIx32", exp_id %”PRId32,
The terminology ambiguity comment applies here as well (exp ID vs. exp type).
> + msg,
> + ofpprop_type_to_experimenter(type),
> + ofpprop_type_to_exp_id(type));
> + }
> +
> + /* There's an error OFPBPC_BAD_EXPERIMENTER that we could use for
> + * experimenter IDs that we don't know at all, but that seems like a
> + * difficult distinction and OFPERR_OFPBPC_BAD_EXP_TYPE communicates the
> + * problem quite well. */
> + return (loose ? 0
> + : is_experimenter ? OFPERR_OFPBPC_BAD_EXP_TYPE
> + : OFPERR_OFPBPC_BAD_TYPE);
> +}
> +
> diff --git a/lib/ofp-prop.h b/lib/ofp-prop.h
> index 07d9799..fc0c76d 100644
> --- a/lib/ofp-prop.h
> +++ b/lib/ofp-prop.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014, 2015 Nicira, Inc.
> + * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -20,30 +20,64 @@
> /* OpenFlow 1.3+ property support
> * ==============================
> *
> - * Several OpenFlow 1.3+ messages use properties that take the common form
> - * shown by "struct ofp_prop_header". This module provides support for
> - * serializing and deserializing properties in this format.
> + * Several OpenFlow 1.3+ messages use type-length-value (TLV) properties that
> + * take the common form shown by "struct ofp_prop_header". This module
> + * provides support for serializing and deserializing properties in this
> + * format.
> + *
> + *
> + * Property types
> + * --------------
> + *
> + * This module uses uint64_t values to identify property types:
> + *
> + * - OpenFlow assigns 16-bit type values to its own standardized
> + * properties. ofpprop uses these values directly in uint64_t.
> + *
Maybe mention that in some cases high 16-bit values denote experimenter types?
> + * - Vendor-specific "experimenter" properties have a 32-bit
> "experimenter
> + * ID" (generally an Ethernet OUI) and a 32-bit experimenter-defined
> + * "exp_type". ofpprop encodes these with the experimenter ID in the
> + * high 32 bits and exp_type in the low 32 bits. (All existing
> + * experimenter IDs are nonzero, so this is unambiguous.) Use
> + * OFPPROP_EXP to encode these property types.
> */
>
> +#include <stdbool.h>
> #include <stddef.h>
> #include <stdint.h>
> #include "ofp-errors.h"
> #include "openvswitch/types.h"
>
> struct ofpbuf;
> +struct vlog_module;
> +
> +/* Given an OpenFlow experimenter ID (e.g. NX_VENDOR_ID) and exp_type, yields
> + * the code that ofpprop_pull() would use to identify the given experimenter
> + * property. */
> +#define OFPPROP_EXP(EXPERIMENTER, EXP_TYPE) \
> + (((uint64_t) (EXPERIMENTER) << 32) | (EXP_TYPE))
> +
> +/* Returns true if 'type' represents an experimenter property type,
> + * false if it represents a standard property type.*/
> +static inline bool
> +ofpprop_is_experimenter(uint64_t type)
> +{
> + return type > UINT16_MAX;
> +}
>
> /* Deserializing properties. */
> enum ofperr ofpprop_pull__(struct ofpbuf *msg, struct ofpbuf *property,
> - unsigned int alignment, uint16_t *typep);
> + unsigned int alignment, unsigned int min_exp,
> + uint64_t *typep);
> enum ofperr ofpprop_pull(struct ofpbuf *msg, struct ofpbuf *property,
> - uint16_t *typep);
> + uint64_t *typep);
>
> /* Serializing properties. */
> -void ofpprop_put(struct ofpbuf *, uint16_t type,
> +void ofpprop_put(struct ofpbuf *, uint64_t type,
> const void *value, size_t len);
> -void ofpprop_put_bitmap(struct ofpbuf *, uint16_t type, uint64_t bitmap);
> +void ofpprop_put_bitmap(struct ofpbuf *, uint64_t type, uint64_t bitmap);
>
> -size_t ofpprop_start(struct ofpbuf *, uint16_t type);
> +size_t ofpprop_start(struct ofpbuf *, uint64_t type);
> void ofpprop_end(struct ofpbuf *, size_t start_ofs);
>
> /* Logging errors while deserializing properties.
> @@ -68,4 +102,9 @@ void ofpprop_end(struct ofpbuf *, size_t start_ofs);
> #define OFPPROP_LOG(RL, LOOSE, ...) \
> VLOG_RL(RL, (LOOSE) ? VLL_DBG : VLL_WARN, __VA_ARGS__)
>
> +enum ofperr ofpprop_unknown(struct vlog_module *, bool loose, const char
> *msg,
> + uint64_t type);
> +#define OFPPROP_UNKNOWN(LOOSE, MSG, TYPE) \
> + ofpprop_unknown(THIS_MODULE, LOOSE, MSG, TYPE)
> +
> #endif /* ofp-prop.h */
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 05cc6d1..449f383 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3785,7 +3785,7 @@ ofputil_pull_ofp14_port(struct ofputil_phy_port *pp,
> struct ofpbuf *msg)
> while (properties.size > 0) {
> struct ofpbuf payload;
> enum ofperr error;
> - uint16_t type;
> + uint64_t type;
>
> error = ofpprop_pull(&properties, &payload, &type);
> if (error) {
> @@ -3798,9 +3798,7 @@ ofputil_pull_ofp14_port(struct ofputil_phy_port *pp,
> struct ofpbuf *msg)
> break;
>
> default:
> - OFPPROP_LOG(&bad_ofmsg_rl, true,
> - "unknown port property %"PRIu16, type);
> - error = 0;
> + error = OFPPROP_UNKNOWN(true, "port", type);
> break;
> }
>
> @@ -4390,7 +4388,7 @@ ofputil_decode_port_mod(const struct ofp_header *oh,
> while (b.size > 0) {
> struct ofpbuf property;
> enum ofperr error;
> - uint16_t type;
> + uint64_t type;
>
> error = ofpprop_pull(&b, &property, &type);
> if (error) {
> @@ -4403,15 +4401,7 @@ ofputil_decode_port_mod(const struct ofp_header *oh,
> break;
>
> default:
> - OFPPROP_LOG(&bad_ofmsg_rl, loose,
> - "unknown port_mod property %"PRIu16, type);
> - if (loose) {
> - error = 0;
> - } else if (type == OFPPMPT14_EXPERIMENTER) {
> - error = OFPERR_OFPBPC_BAD_EXPERIMENTER;
> - } else {
> - error = OFPERR_OFPBRC_BAD_TYPE;
> - }
> + error = OFPPROP_UNKNOWN(loose, "port_mod", type);
> break;
> }
>
> @@ -4496,13 +4486,13 @@ ofputil_encode_port_mod(const struct ofputil_port_mod
> *pm,
>
> static enum ofperr
> pull_table_feature_property(struct ofpbuf *msg, struct ofpbuf *payload,
> - uint16_t *typep)
> + uint64_t *typep)
> {
> enum ofperr error;
>
> error = ofpprop_pull(msg, payload, typep);
> if (payload && !error) {
> - ofpbuf_pull(payload, sizeof(struct ofp_prop_header));
> + ofpbuf_pull(payload, (uint8_t *)msg->msg - (uint8_t *)msg->header);
> }
> return error;
> }
> @@ -4514,10 +4504,10 @@ parse_action_bitmap(struct ofpbuf *payload, enum
> ofp_version ofp_version,
> uint32_t types = 0;
>
> while (payload->size > 0) {
> - uint16_t type;
> enum ofperr error;
> + uint64_t type;
>
> - error = ofpprop_pull__(payload, NULL, 1, &type);
> + error = ofpprop_pull__(payload, NULL, 1, 0x10000, &type);
It would be nice to document the significance of the value ‘0x10000’ here.
> if (error) {
> return error;
> }
> @@ -4537,7 +4527,7 @@ parse_instruction_ids(struct ofpbuf *payload, bool
> loose, uint32_t *insts)
> while (payload->size > 0) {
> enum ovs_instruction_type inst;
> enum ofperr error;
> - uint16_t ofpit;
> + uint64_t ofpit;
>
(snip)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
