On Aug 28, 2013, at 11:55 PM, Ben Pfaff <b...@nicira.com> wrote:
> From: Neil Zhu <z...@centecnetworks.com>
>
> This doesn't include a dpif implementation of groups functionality. In its
> current form, it is untested. Before this is committed, it needs some
> more comments and an ovs-ofctl manpage update.
>
> Signed-off-by: Neil Zhu <z...@centecnetworks.com>
> Co-authored-by: Ben Pfaff <b...@nicira.com>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
I did a review and some initial testing. I'll send an incremental patch
separately for your review. Its all mainly just style issues, but I pointed
out some real issues below.
...
> @@ -1794,6 +1799,7 @@ parse_ofp_flow_stats_request_str(struct
> ofputil_flow_stats_request *fsr,
> fsr->cookie_mask = fm.cookie_mask;
> fsr->match = fm.match;
> fsr->out_port = fm.out_port;
> + fsr->out_group = fm.out_group;
> fsr->table_id = fm.table_id;
> return NULL;
> }
> @@ -1878,3 +1884,257 @@ exit:
> }
> return error;
> }
> +
> +static char * WARN_UNUSED_RESULT
> +parse_bucket_str(struct ofputil_bucket *p_bucket, char *str_,
> + enum ofputil_protocol *usable_protocols)
> +{
> + struct ofpbuf ofpacts;
> + char *pos, *act, *arg;
> + int n_actions;
> +
> + p_bucket->weight = 1;
> + p_bucket->watch_port = OFPP_ANY;
> + p_bucket->watch_group = OFPG11_ANY;
> +
> + pos = str_;
> + n_actions = 0;
> + ofpbuf_init(&ofpacts, 64);
> + while (ofputil_parse_key_value(&pos, &act, &arg)) {
> + char *error = NULL;
> +
> + if (!strcasecmp(act, "weight")) {
The rest to the file only uses strcmp(), not strcasecmp(), except for
actions. I changed these to strcmp()s.
...
> +static char * WARN_UNUSED_RESULT
> +parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
> + char *string,
> + enum ofputil_protocol *usable_protocols)
> +{
> + enum {
> + F_GROUP_TYPE = 1 << 0,
> + F_BUCKETS = 1 << 1,
> + } fields;
> + char *save_ptr = NULL;
> + char *bkt_str = NULL;
> + char *next_bkt_str = NULL;
> + bool had_type = false;
> + char *name;
> +
> + *usable_protocols = OFPUTIL_P_OF11_UP;
> +
> + switch (command) {
> + case OFPGC11_ADD:
> + fields = F_GROUP_TYPE | F_BUCKETS;
> + break;
> +
> + case OFPGC11_DELETE:
> + fields = 0;
> + break;
> +
> + case OFPGC11_MODIFY:
> + fields = F_GROUP_TYPE | F_BUCKETS;
> + break;
> +
> + default:
> + NOT_REACHED();
> + }
> +
> + memset(&gm, 0, sizeof gm);
This needs to be memset(gm, 0, sizeof *gm) instead. Spotted via
"Illegal instruction" signal...
> + gm->command = command;
> + gm->group_id = OFPG_ANY;
> + list_init(&gm->buckets);
> + if (command == OFPGC11_DELETE && string[0] == '\0') {
> + gm->group_id = OFPG_ALL;
> + return NULL;
> + }
> +
> + *usable_protocols = OFPUTIL_P_OF11_UP;
> +
> + bkt_str = strstr(string, "bucket");
> + if (fields & F_BUCKETS && bkt_str) {
> + struct ofputil_bucket *p_bucket = NULL;
> +
> + *bkt_str = '\0';
> +
> + while (bkt_str) {
> + char *error;
> +
> + bkt_str = strchr(bkt_str + 1, '=');
> + if (!bkt_str) {
> + return xstrdup("must specify bucket content");
Here (and elsewhere) already allocated buckets are not freed.
The incremental addresses this.
...
> +int
> +ofputil_decode_group_stats_reply(struct ofpbuf *msg,
> + struct ofputil_group_stats *ogs)
> +{
> + struct ofp11_bucket_counter *obc;
> + struct ofp11_group_stats *ogs11;
> + enum ofpraw raw;
> + enum ofperr error;
> + size_t base_len;
> + size_t length;
> + size_t i;
> +
> + if (!msg->size) {
> + return EOF;
> + }
> +
> + error = (msg->l2
> + ? ofpraw_decode(&raw, msg->l2)
> + : ofpraw_pull(&raw, msg));
> + if (error) {
> + return error;
> + }
> +
> + if (raw == OFPRAW_OFPST11_GROUP_REPLY) {
> + base_len = sizeof *ogs11;
> + ogs11 = ofpbuf_try_pull(msg, sizeof *ogs11);
> + ogs->duration_sec = ogs->duration_nsec = UINT32_MAX;
> + } else if (raw == OFPRAW_OFPST13_GROUP_REPLY) {
> + struct ofp13_group_stats *ogs13;
> +
> + base_len = sizeof *ogs13;
> + ogs13 = ofpbuf_try_pull(msg, sizeof *ogs13);
> + if (ogs13) {
> + ogs11 = &ogs13->gs;
> + ogs->duration_sec = ntohl(ogs13->duration_sec);
> + ogs->duration_nsec = ntohl(ogs13->duration_nsec);
> + } else {
> + ogs11 = NULL;
> + }
> + } else {
> + NOT_REACHED();
> + }
> +
> + ogs->group_id = ntohl(ogs11->group_id);
> + ogs->ref_count = ntohl(ogs11->ref_count);
> + ogs->packet_count = ntohll(ogs11->packet_count);
> + ogs->byte_count = ntohll(ogs11->byte_count);
> +
Here ogs11 may be NULL, so these need to be moved down a bit.
> + if (!ogs11) {
> + VLOG_WARN_RL(&bad_ofmsg_rl, "%s reply has %zu leftover bytes at end",
> + ofpraw_get_name(raw), msg->size);
> + return OFPERR_OFPBRC_BAD_LEN;
> + }
> + length = ntohs(ogs11->length);
> + if (length < sizeof base_len) {
> + VLOG_WARN_RL(&bad_ofmsg_rl, "%s reply claims invalid length %zu",
> + ofpraw_get_name(raw), length);
> + return OFPERR_OFPBRC_BAD_LEN;
> + }
> +
>
...
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 820ec34..cfbc240 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -6384,4 +6384,11 @@ const struct ofproto_class ofproto_dpif_class = {
> NULL, /* meter_set */
> NULL, /* meter_get */
> NULL, /* meter_del */
> + NULL, /* group_alloc */
> + NULL, /* group_construct */
> + NULL, /* group_destruct */
> + NULL, /* group_dealloc */
> + NULL, /* group_modify */
> + NULL, /* group_get_stats */
> + NULL, /* group_get_ref_cnt */
I do not see the value of having a separate function for
group_get_ref_cnt, see below:
...
> +static void
> +append_group_stats(struct ofgroup *group, struct list *replies)
> +{
> + struct ofputil_group_stats ogs;
> + struct ofproto *ofproto = group->ofproto;
> + long long int now = time_msec();
> + int error;
> +
> + error = (ofproto->ofproto_class->group_get_stats
> + ? ofproto->ofproto_class->group_get_stats(group, &ogs)
> + : EOPNOTSUPP);
> + if (error) {
> + ogs.packet_count = UINT64_MAX;
> + ogs.byte_count = UINT64_MAX;
> + ogs.n_buckets = group->n_buckets;
> + memset(ogs.bucket_stats, 0xff,
> + ogs.n_buckets * sizeof *ogs.bucket_stats);
> + }
> +
> + error = (ofproto->ofproto_class->group_get_ref_cnt
> + ? ofproto->ofproto_class->group_get_ref_cnt(group,
> &ogs.ref_count)
> + : EOPNOTSUPP);
> + if (error) {
> + ogs.ref_count = UINT32_MAX;
> + }
> +
It would be simper just to have the group_get_stats return also the
ref_cnt, which is part of the stats structure anyway. In case the ofproto
provider does not support ref_cnt, it can return it as UINT32_MAX to
retain the behavior here.
Also, it seems to me that the reference counting should be implemented
in the ofproto proper anyway, rather than in an ofproto-provider. In the
incremental I have removed the group_get_ref_cnt().
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
