[ovs-dev] [RFC PATCH 3/8] ofp-util: Support for OpenFlow 1.3 meters.

Rajahalme, Jarno (NSN - FI/Espoo) jarno.rajahalme at nsn.com
Wed Jun 26 22:47:22 UTC 2013


On Jun 25, 2013, at 20:35 , ext Ben Pfaff wrote:

> On Thu, Jun 20, 2013 at 05:26:18PM +0300, Jarno Rajahalme wrote:
>> 
>> Signed-off-by: Jarno Rajahalme <jarno.rajahalme at nsn.com>
> 
> I'm applied this to master.  I'm appending the incremental that I
> folded in.  Most of the changes are minor improvements (some just
> stylistic).  The only important change is that it looked to me like
> the OpenFlow parsing code in ofp-util.c didn't check that the lengths
> were valid, so I adjusted them so that they did check.
> 

Thanks for doing the fixes rather than bouncing them back at me :-) I noticed some of these while reviewing the other meters patch (like the ofp_print_duration() that I had not noticed before).

> I noticed that the tests seem to use impossible numbers of nanoseconds
> (greater than 1e9), but I didn't change that other than the
> formatting.

Oops, I just put some random numbers there. Will fix in a later patch.

  Jarno

> 
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index f1f755b..f06edb1 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -48,12 +48,12 @@ static void ofp_fatal(const char *flow, bool verbose, const char *format, ...)
> static uint8_t
> str_to_u8(const char *str, const char *name)
> {
> -    int table_id;
> +    int value;
> 
> -    if (!str_to_int(str, 10, &table_id) || table_id < 0 || table_id > 255) {
> +    if (!str_to_int(str, 10, &value) || value < 0 || value > 255) {
>         ovs_fatal(0, "invalid %s \"%s\"", name, str);
>     }
> -    return table_id;
> +    return value;
> }
> 
> static uint16_t
> @@ -373,16 +373,6 @@ set_field_parse(const char *arg, struct ofpbuf *ofpacts)
> }
> 
> static void
> -parse_meter(struct ofpbuf *b, char *arg)
> -{
> -    struct ofpact_meter *om;
> -
> -    om = ofpact_put_METER(b);
> -
> -    om->meter_id = str_to_u32(arg);
> -}
> -
> -static void
> parse_metadata(struct ofpbuf *b, char *arg)
> {
>     struct ofpact_metadata *om;
> @@ -717,7 +707,7 @@ parse_named_instruction(enum ovs_instruction_type type,
>         break;
> 
>     case OVSINST_OFPIT13_METER:
> -        parse_meter(ofpacts, arg);
> +        ofpact_put_METER(ofpacts)->meter_id = str_to_u32(arg);
>         break;
> 
>     case OVSINST_OFPIT11_WRITE_METADATA:
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index e554828..76bd09c 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -1013,8 +1013,9 @@ ofp_print_meter_stats(struct ds *s, const struct ofputil_meter_stats *ms)
>     ds_put_format(s, "flow_count:%"PRIu32" ", ms->flow_count);
>     ds_put_format(s, "packet_in_count:%"PRIu64" ", ms->packet_in_count);
>     ds_put_format(s, "byte_in_count:%"PRIu64" ", ms->byte_in_count);
> -    ds_put_format(s, "duration_sec:%"PRIu32" ", ms->duration_sec);
> -    ds_put_format(s, "duration_nsec:%"PRIu32" ", ms->duration_nsec);
> +    ds_put_cstr(s, "duration:");
> +    ofp_print_duration(s, ms->duration_sec, ms->duration_nsec);
> +    ds_put_char(s, ' ');
> 
>     ds_put_cstr(s, "bands:\n");
>     for (i = 0; i < ms->n_bands; ++i) {
> @@ -1151,7 +1152,6 @@ ofp_print_meter_config_reply(struct ds *s, const struct ofp_header *oh)
>         if (retval) {
>             if (retval != EOF) {
>                 ofp_print_error(s, retval);
> -                /* ds_put_cstr(s, " ***parse error***"); */
>             }
>             break;
>         }
> @@ -1177,7 +1177,6 @@ ofp_print_meter_stats_reply(struct ds *s, const struct ofp_header *oh)
>         if (retval) {
>             if (retval != EOF) {
>                 ofp_print_error(s, retval);
> -                /* ds_put_cstr(s, " ***parse error***"); */
>             }
>             break;
>         }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 461e138..6c58415 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1639,7 +1639,10 @@ ofputil_pull_bands(struct ofpbuf *msg, size_t len, uint16_t *n_bands,
>     struct ofputil_meter_band *mb;
>     uint16_t n = 0;
> 
> -    ombh = ofpbuf_pull(msg, len);
> +    ombh = ofpbuf_try_pull(msg, len);
> +    if (!ombh) {
> +        return OFPERR_OFPBRC_BAD_LEN;
> +    }
> 
>     while (len >= sizeof (struct ofp13_meter_band_drop)) {
>         size_t ombh_len = ntohs(ombh->len);
> @@ -1669,7 +1672,12 @@ ofputil_decode_meter_mod(const struct ofp_header *oh,
>                          struct ofputil_meter_mod *mm,
>                          struct ofpbuf *bands)
> {
> -    const struct ofp13_meter_mod *omm = ofpmsg_body(oh);
> +    const struct ofp13_meter_mod *omm;
> +    struct ofpbuf b;
> +
> +    ofpbuf_use_const(&b, oh, ntohs(oh->length));
> +    ofpraw_pull_assert(&b);
> +    omm = ofpbuf_pull(&b, sizeof *omm);
> 
>     /* Translate the message. */
>     mm->command = ntohs(omm->command);
> @@ -1680,15 +1688,11 @@ ofputil_decode_meter_mod(const struct ofp_header *oh,
>         mm->meter.n_bands = 0;
>         mm->meter.bands = NULL;
>     } else {
> -        struct ofpbuf b;
>         enum ofperr error;
> 
>         mm->meter.flags = ntohs(omm->flags);
>         mm->meter.bands = bands->data;
> 
> -        ofpbuf_use_const(&b, omm + 1,
> -                         ntohs(oh->length) - sizeof *oh - sizeof *omm);
> -
>         error = ofputil_pull_bands(&b, b.size, &mm->meter.n_bands,
>                                    bands);
>         if (error) {
> @@ -1727,7 +1731,7 @@ ofputil_encode_meter_request(enum ofp_version ofp_version,
>         break;
>     }
> 
> -    msg = ofpraw_alloc(raw, MAX(OFP13_VERSION, ofp_version), 0);
> +    msg = ofpraw_alloc(raw, ofp_version, 0);
> 
>     if (type != OFPUTIL_METER_FEATURES) {
>         struct ofp13_meter_multipart_request *omr;
> @@ -1741,20 +1745,20 @@ static void
> ofputil_put_bands(uint16_t n_bands, const struct ofputil_meter_band *mb,
>                   struct ofpbuf *msg)
> {
> -    struct ofp13_meter_band_dscp_remark *ombh;
>     uint16_t n = 0;
> 
>     for (n = 0; n < n_bands; ++n) {
>         /* Currently all band types have same size */
> +        struct ofp13_meter_band_dscp_remark *ombh;
>         size_t ombh_len = sizeof *ombh;
> -        ombh = ofpbuf_put_uninit(msg, ombh_len);
> +
> +        ombh = ofpbuf_put_zeros(msg, ombh_len);
> 
>         ombh->type = htons(mb->type);
>         ombh->len = htons(ombh_len);
>         ombh->rate = htonl(mb->rate);
>         ombh->burst_size = htonl(mb->burst_size);
>         ombh->prec_level = mb->prec_level;
> -        memset(ombh->pad, 0, sizeof ombh->pad);
> 
>         mb++;
>     }
> @@ -1800,15 +1804,18 @@ ofputil_append_meter_stats(struct list *replies,
>     reply->duration_nsec = htonl(ms->duration_nsec);
> 
>     for (n = 0; n < ms->n_bands; ++n) {
> -        reply->band_stats[n].packet_band_count
> -            = htonll(ms->bands[n].packet_count);
> -        reply->band_stats[n].byte_band_count = htonll(ms->bands[n].byte_count);
> +        const struct ofputil_meter_band_stats *src = &ms->bands[n];
> +        struct ofp13_meter_band_stats *dst = &reply->band_stats[n];
> +
> +        dst->packet_band_count = htonll(src->packet_count);
> +        dst->byte_band_count = htonll(src->byte_count);
>     }
> }
> 
> /* Converts an OFPMP_METER_CONFIG reply in 'msg' into an abstract
> - * ofputil_meter_config in 'mc', with mc->bands pointing to bands
> - * decoded into 'bands'.
> + * ofputil_meter_config in 'mc', with mc->bands pointing to bands decoded into
> + * 'bands'.  The caller must have initialized 'bands' and retains ownership of
> + * it across the call.
>  *
>  * Multiple OFPST13_METER_CONFIG replies can be packed into a single OpenFlow
>  * message.  Calling this function multiple times for a single 'msg' iterates
> @@ -1822,12 +1829,10 @@ ofputil_decode_meter_config(struct ofpbuf *msg,
>                             struct ofpbuf *bands)
> {
>     const struct ofp13_meter_config *omc;
> -    uint16_t len;
>     enum ofperr err;
> 
> -    /* pull OpenFlow headers for the first call */
> +    /* Pull OpenFlow headers for the first call. */
>     if (!msg->l2) {
> -        msg->l2 = msg->data;
>         ofpraw_pull_assert(msg);
>     }
> 
> @@ -1837,28 +1842,22 @@ ofputil_decode_meter_config(struct ofpbuf *msg,
> 
>     omc = ofpbuf_try_pull(msg, sizeof *omc);
>     if (!omc) {
> -        goto bad_len;
> +        VLOG_WARN_RL(&bad_ofmsg_rl, "OFPMP_METER_CONFIG reply has %zu "
> +                     "leftover bytes at end", msg->size);
> +        return OFPERR_OFPBRC_BAD_LEN;
>     }
> -    len = ntohs(omc->length);
> -    len -= sizeof *omc;
> 
>     ofpbuf_clear(bands);
> -    err = ofputil_pull_bands(msg, len, &mc->n_bands, bands);
> +    err = ofputil_pull_bands(msg, ntohs(omc->length) - sizeof *omc,
> +                             &mc->n_bands, bands);
>     if (err) {
> -        goto error;
> +        return err;
>     }
>     mc->meter_id = ntohl(omc->meter_id);
>     mc->flags = ntohs(omc->flags);
>     mc->bands = bands->data;
> 
>     return 0;
> -
> - bad_len:
> -    err = OFPERR_OFPBRC_BAD_LEN;
> -    VLOG_WARN_RL(&bad_ofmsg_rl, "OFPMP_METER_CONFIG reply has %zu leftover "
> -                 "bytes at end", msg->size);
> - error:
> -    return err;
> }
> 
> static enum ofperr
> @@ -1905,9 +1904,8 @@ ofputil_decode_meter_stats(struct ofpbuf *msg,
>     uint16_t len;
>     enum ofperr err;
> 
> -    /* pull OpenFlow headers for the first call */
> +    /* Pull OpenFlow headers for the first call. */
>     if (!msg->l2) {
> -        msg->l2 = msg->data;
>         ofpraw_pull_assert(msg);
>     }
> 
> @@ -1917,7 +1915,9 @@ ofputil_decode_meter_stats(struct ofpbuf *msg,
> 
>     oms = ofpbuf_try_pull(msg, sizeof *oms);
>     if (!oms) {
> -        goto bad_len;
> +        VLOG_WARN_RL(&bad_ofmsg_rl, "OFPMP_METER reply has %zu leftover "
> +                     "bytes at end", msg->size);
> +        return OFPERR_OFPBRC_BAD_LEN;
>     }
>     len = ntohs(oms->len);
>     len -= sizeof *oms;
> @@ -1925,7 +1925,7 @@ ofputil_decode_meter_stats(struct ofpbuf *msg,
>     ofpbuf_clear(bands);
>     err = ofputil_pull_band_stats(msg, len, &ms->n_bands, bands);
>     if (err) {
> -        goto error;
> +        return err;
>     }
>     ms->meter_id = ntohl(oms->meter_id);
>     ms->flow_count = ntohl(oms->flow_count);
> @@ -1936,13 +1936,6 @@ ofputil_decode_meter_stats(struct ofpbuf *msg,
>     ms->bands = bands->data;
> 
>     return 0;
> -
> - bad_len:
> -    err = OFPERR_OFPBRC_BAD_LEN;
> -    VLOG_WARN_RL(&bad_ofmsg_rl, "OFPMP_METER reply has %zu leftover "
> -                 "bytes at end", msg->size);
> - error:
> -    return err;
> }
> 
> void
> @@ -1985,8 +1978,7 @@ ofputil_encode_meter_mod(enum ofp_version ofp_version,
> 
>     struct ofp13_meter_mod *omm;
> 
> -    msg = ofpraw_alloc(OFPRAW_OFPT13_METER_MOD,
> -                       MAX(OFP13_VERSION, ofp_version), /* FIXME? */
> +    msg = ofpraw_alloc(OFPRAW_OFPT13_METER_MOD, ofp_version,
>                        NXM_TYPICAL_LEN + mm->meter.n_bands * 16);
>     omm = ofpbuf_put_zeros(msg, sizeof *omm);
>     omm->command = htons(mm->command);
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> index d6cfdf8..b3e3eb5 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -1661,11 +1661,11 @@ AT_CHECK([ovs-ofctl ofp-print "\
> 00 00 00 00 00 00 00 2a 00 00 00 00 00 00 04 33 \
> "], [0], [dnl
> OFPST_METER reply (OF1.3) (xid=0x2):
> -meter:1 flow_count:5 packet_in_count:4096 byte_in_count:143360 duration_sec:394 duration_nsec:2590909252 bands:
> +meter:1 flow_count:5 packet_in_count:4096 byte_in_count:143360 duration:394.2590909252s bands:
> 0: packet_count:126 byte_count:13363
> 1: packet_count:231 byte_count:37934
> 
> -meter:2 flow_count:2 packet_in_count:512 byte_in_count:12288 duration_sec:391 duration_nsec:2586013252 bands:
> +meter:2 flow_count:2 packet_in_count:512 byte_in_count:12288 duration:391.2586013252s bands:
> 0: packet_count:42 byte_count:1075
> ])
> AT_CLEANUP




More information about the dev mailing list