This is a set of proposed modifications to the patch "groups v2 2/2".
Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
---
lib/ofp-parse.c | 111 +++++++++++++++++++++++++++-----------------
lib/ofp-print.c | 9 ++--
lib/ofp-util.c | 70 +++++++++++++++-------------
ofproto/ofproto-dpif.c | 1 -
ofproto/ofproto-provider.h | 2 -
ofproto/ofproto.c | 8 +---
tests/ofp-print.at | 4 +-
utilities/ovs-ofctl.c | 6 +--
8 files changed, 115 insertions(+), 96 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index ae43068..9098467 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1886,16 +1886,16 @@ exit:
}
static char * WARN_UNUSED_RESULT
-parse_bucket_str(struct ofputil_bucket *p_bucket, char *str_,
+parse_bucket_str(struct ofputil_bucket *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;
+ bucket->weight = 1;
+ bucket->watch_port = OFPP_ANY;
+ bucket->watch_group = OFPG11_ANY;
pos = str_;
n_actions = 0;
@@ -1904,15 +1904,19 @@ parse_bucket_str(struct ofputil_bucket *p_bucket, char
*str_,
char *error = NULL;
if (!strcasecmp(act, "weight")) {
- error = str_to_u16(arg, "weight", &p_bucket->weight);
+ error = str_to_u16(arg, "weight", &bucket->weight);
} else if (!strcasecmp(act, "watch_port")) {
- if (!ofputil_port_from_string(arg, &p_bucket->watch_port)
- || (ofp_to_u16(p_bucket->watch_port) >= ofp_to_u16(OFPP_MAX)
- && p_bucket->watch_port != OFPP_ANY)) {
+ if (!ofputil_port_from_string(arg, &bucket->watch_port)
+ || (ofp_to_u16(bucket->watch_port) >= ofp_to_u16(OFPP_MAX)
+ && bucket->watch_port != OFPP_ANY)) {
error = xasprintf("%s: invalid watch_port", arg);
}
} else if (!strcasecmp(act, "watch_group")) {
- error = str_to_u32(arg, &p_bucket->watch_group);
+ error = str_to_u32(arg, &bucket->watch_group);
+ if (!error && bucket->watch_group > OFPG_MAX) {
+ error = xasprintf("invalid watch_group id %"PRIu32,
+ bucket->watch_group);
+ }
} else {
error = str_to_ofpact__(pos, act, arg, &ofpacts, n_actions,
usable_protocols);
@@ -1926,8 +1930,8 @@ parse_bucket_str(struct ofputil_bucket *p_bucket, char
*str_,
}
ofpact_pad(&ofpacts);
- p_bucket->ofpacts = ofpacts.data;
- p_bucket->ofpacts_len = ofpacts.size;
+ bucket->ofpacts = ofpacts.data;
+ bucket->ofpacts_len = ofpacts.size;
return NULL;
}
@@ -1942,10 +1946,10 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm,
uint16_t command,
F_BUCKETS = 1 << 1,
} fields;
char *save_ptr = NULL;
- char *bkt_str = NULL;
- char *next_bkt_str = NULL;
bool had_type = false;
char *name;
+ struct ofputil_bucket *bucket;
+ char *error = NULL;
*usable_protocols = OFPUTIL_P_OF11_UP;
@@ -1966,7 +1970,7 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm,
uint16_t command,
NOT_REACHED();
}
- memset(&gm, 0, sizeof gm);
+ memset(gm, 0, sizeof *gm);
gm->command = command;
gm->group_id = OFPG_ANY;
list_init(&gm->buckets);
@@ -1977,18 +1981,20 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm,
uint16_t command,
*usable_protocols = OFPUTIL_P_OF11_UP;
- bkt_str = strstr(string, "bucket");
- if (fields & F_BUCKETS && bkt_str) {
- struct ofputil_bucket *p_bucket = NULL;
+ if (fields & F_BUCKETS) {
+ char *bkt_str = strstr(string, "bucket");
- *bkt_str = '\0';
+ if (bkt_str) {
+ *bkt_str = '\0';
+ }
while (bkt_str) {
- char *error;
+ char *next_bkt_str;
bkt_str = strchr(bkt_str + 1, '=');
if (!bkt_str) {
- return xstrdup("must specify bucket content");
+ error = xstrdup("must specify bucket content");
+ goto out;
}
bkt_str++;
@@ -1997,13 +2003,13 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm,
uint16_t command,
*next_bkt_str = '\0';
}
- p_bucket = xzalloc(sizeof(struct ofputil_bucket));
- error = parse_bucket_str(p_bucket, bkt_str, usable_protocols);
+ bucket = xzalloc(sizeof(struct ofputil_bucket));
+ error = parse_bucket_str(bucket, bkt_str, usable_protocols);
if (error) {
- free(p_bucket);
- return error;
+ free(bucket);
+ goto out;
}
- list_push_back(&gm->buckets, &p_bucket->list_node);
+ list_push_back(&gm->buckets, &bucket->list_node);
bkt_str = next_bkt_str;
}
@@ -2015,52 +2021,71 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm,
uint16_t command,
value = strtok_r(NULL, ", \t\r\n", &save_ptr);
if (!value) {
- return xasprintf("field %s missing value", name);
+ error = xasprintf("field %s missing value", name);
+ goto out;
}
- if (!strcasecmp(name, "group_id")) {
- if(!strcasecmp(value, "all")) {
+ if (!strcmp(name, "group_id")) {
+ if(!strcmp(value, "all")) {
gm->group_id = OFPG_ALL;
} else {
char *error = str_to_u32(value, &gm->group_id);
if (error) {
- return error;
+ goto out;
}
if (gm->group_id != OFPG_ALL && gm->group_id > OFPG_MAX) {
- return xasprintf("invalid group id %"PRIu32, gm->group_id);
+ error = xasprintf("invalid group id %"PRIu32,
+ gm->group_id);
+ goto out;
}
}
- } else if (!strcasecmp(name, "type")){
+ } else if (!strcmp(name, "type")){
if (!(fields & F_GROUP_TYPE)) {
- return xstrdup("type is not needed");
+ error = xstrdup("type is not needed");
+ goto out;
}
- if (!strcasecmp(value, "all")) {
+ if (!strcmp(value, "all")) {
gm->type = OFPGT11_ALL;
- } else if (!strcasecmp(value, "select")) {
+ } else if (!strcmp(value, "select")) {
gm->type = OFPGT11_SELECT;
- } else if (!strcasecmp(value, "indirect")) {
+ } else if (!strcmp(value, "indirect")) {
gm->type = OFPGT11_INDIRECT;
- } else if (!strcasecmp(value, "ff") ||
- !strcasecmp(value, "fast_failover")) {
+ } else if (!strcmp(value, "ff") ||
+ !strcmp(value, "fast_failover")) {
gm->type = OFPGT11_FF;
} else {
- return xasprintf("invalid group type %s", value);
+ error = xasprintf("invalid group type %s", value);
+ goto out;
}
had_type = true;
- } else if (!strcasecmp(name, "bucket")) {
- return xstrdup("bucket is not needed");
+ } else if (!strcmp(name, "bucket")) {
+ error = xstrdup("bucket is not needed");
+ goto out;
} else {
- return xasprintf("unknown keyword %s", name);
+ error = xasprintf("unknown keyword %s", name);
+ goto out;
}
}
if (gm->group_id == OFPG_ANY) {
- return xstrdup("must specify a group_id");
+ error = xstrdup("must specify a group_id");
+ goto out;
}
if (fields & F_GROUP_TYPE && !had_type) {
- return xstrdup("must specify a type");
+ error = xstrdup("must specify a type");
+ goto out;
+ }
+ /* Validate buckets. */
+ LIST_FOR_EACH(bucket, list_node, &gm->buckets) {
+ if (bucket->weight != 1 && gm->type != OFPGT11_SELECT) {
+ error = xstrdup("Only select groups can have bucket weights.");
+ goto out;
+ }
}
return NULL;
+ out:
+ ofputil_bucket_list_destroy(&gm->buckets);
+ return error;
}
char * WARN_UNUSED_RESULT
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 2c2cb3a..cc1bd73 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2144,12 +2144,12 @@ static void
ofp_print_group(struct ds *s, uint32_t group_id, uint8_t type,
struct list *p_buckets)
{
- static const char *type_str[] = { "ALL", "SELECT", "INDIRECT",
- "FF", "UNKNOWN" };
+ static const char *type_str[] = { "all", "select", "indirect",
+ "ff", "unknown" };
struct ofputil_bucket *bucket;
ds_put_format(s, "group_id=%"PRIu32",type=%s",
- group_id, type_str[type>3?4:type]);
+ group_id, type_str[type > 4 ? 4 : type]);
if (!p_buckets) {
return;
}
@@ -2167,7 +2167,6 @@ ofp_print_group(struct ds *s, uint32_t group_id, uint8_t
type,
ds_put_format(s, "watch_group:%"PRIu32",", bucket->watch_group);
}
- ds_put_cstr(s, "actions=");
ofpacts_format(bucket->ofpacts, bucket->ofpacts_len, s);
}
}
@@ -2216,7 +2215,7 @@ static void
ofp_print_group_stats(struct ds *s, const struct ofp_header *oh)
{
struct ofpbuf b;
- size_t bucket_i;
+ uint32_t bucket_i;
ofpbuf_use_const(&b, oh, ntohs(oh->length));
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 91da5f0..492c494 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5275,9 +5275,9 @@ ofputil_encode_group_stats_request(enum ofp_version
ofp_version,
struct ofpbuf *request;
switch (ofp_version) {
- case OFP10_VERSION: {
- ovs_fatal(0, "dump-gstats need argument \'-O openflow11(+)\'");
- }
+ case OFP10_VERSION:
+ ovs_fatal(0, "dump-group-stats needs OpenFlow 1.1 or later "
+ "(\'-O OpenFlow11\')");
case OFP11_VERSION:
case OFP12_VERSION:
case OFP13_VERSION: {
@@ -5300,9 +5300,9 @@ ofputil_encode_group_desc_request(enum ofp_version
ofp_version)
struct ofpbuf *request;
switch (ofp_version) {
- case OFP10_VERSION: {
- ovs_fatal(0, "dump-groups need argument \'-O openflow11(+)\'");
- }
+ case OFP10_VERSION:
+ ovs_fatal(0, "dump-groups needs OpenFlow 1.1 or later "
+ "(\'-O OpenFlow11\')");
case OFP11_VERSION:
case OFP12_VERSION:
case OFP13_VERSION: {
@@ -5389,9 +5389,9 @@ ofputil_encode_group_features_request(enum ofp_version
ofp_version)
switch (ofp_version) {
case OFP10_VERSION:
- case OFP11_VERSION:{
- ovs_fatal(0, "dump-gfeatures need argument \'-O openflow12(+)\'");
- }
+ case OFP11_VERSION:
+ ovs_fatal(0, "dump-group-features needs OpenFlow 1.2 or later "
+ "(\'-O OpenFlow12\')");
case OFP12_VERSION:
case OFP13_VERSION: {
request = ofpraw_alloc(OFPRAW_OFPST12_GROUP_FEATURES_REQUEST,
@@ -5417,8 +5417,8 @@ ofputil_encode_group_features_reply(
switch ((enum ofp_version) request->version) {
case OFP10_VERSION:
case OFP11_VERSION:
- ovs_fatal(0, "Group need argument \'-O Openflow12\'");
-
+ ovs_fatal(0, "Group features reply needs OpenFlow 1.2 or later "
+ "(\'-O OpenFlow12\')");
case OFP12_VERSION:
case OFP13_VERSION:
raw = OFPRAW_OFPST12_GROUP_FEATURES_REPLY;
@@ -5486,7 +5486,7 @@ ofputil_decode_group_stats_request(const struct
ofp_header *request,
int
ofputil_decode_group_stats_reply(struct ofpbuf *msg,
- struct ofputil_group_stats *ogs)
+ struct ofputil_group_stats *gs)
{
struct ofp11_bucket_counter *obc;
struct ofp11_group_stats *ogs11;
@@ -5510,7 +5510,7 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
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;
+ gs->duration_sec = gs->duration_nsec = UINT32_MAX;
} else if (raw == OFPRAW_OFPST13_GROUP_REPLY) {
struct ofp13_group_stats *ogs13;
@@ -5518,8 +5518,8 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
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);
+ gs->duration_sec = ntohl(ogs13->duration_sec);
+ gs->duration_nsec = ntohl(ogs13->duration_nsec);
} else {
ogs11 = NULL;
}
@@ -5527,11 +5527,6 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
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);
-
if (!ogs11) {
VLOG_WARN_RL(&bad_ofmsg_rl, "%s reply has %zu leftover bytes at end",
ofpraw_get_name(raw), msg->size);
@@ -5544,17 +5539,22 @@ ofputil_decode_group_stats_reply(struct ofpbuf *msg,
return OFPERR_OFPBRC_BAD_LEN;
}
- ogs->n_buckets = (length - base_len) / sizeof(struct ofp11_bucket_counter);
- obc = ofpbuf_try_pull(msg, ogs->n_buckets * sizeof *obc);
+ gs->group_id = ntohl(ogs11->group_id);
+ gs->ref_count = ntohl(ogs11->ref_count);
+ gs->packet_count = ntohll(ogs11->packet_count);
+ gs->byte_count = ntohll(ogs11->byte_count);
+
+ gs->n_buckets = (length - base_len) / sizeof *obc;
+ obc = ofpbuf_try_pull(msg, gs->n_buckets * sizeof *obc);
if (!obc) {
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;
}
- for (i = 0; i < ogs->n_buckets; i++) {
- ogs->bucket_stats[i].packet_count = ntohll(obc[i].packet_count);
- ogs->bucket_stats[i].byte_count = ntohll(obc[i].byte_count);
+ for (i = 0; i < gs->n_buckets; i++) {
+ gs->bucket_stats[i].packet_count = ntohll(obc[i].packet_count);
+ gs->bucket_stats[i].byte_count = ntohll(obc[i].byte_count);
}
return 0;
@@ -5659,7 +5659,7 @@ ofputil_pull_buckets(struct ofpbuf *msg, size_t
buckets_length,
}
int
-ofputil_decode_group_desc_reply(struct ofputil_group_desc *ogd,
+ofputil_decode_group_desc_reply(struct ofputil_group_desc *gd,
struct ofpbuf *msg)
{
struct ofp11_group_desc_stats *ogds;
@@ -5679,8 +5679,8 @@ ofputil_decode_group_desc_reply(struct ofputil_group_desc
*ogd,
"leftover bytes at end", msg->size);
return OFPERR_OFPBRC_BAD_LEN;
}
- ogd->type = ogds->type;
- ogd->group_id = ntohl(ogds->group_id);
+ gd->type = ogds->type;
+ gd->group_id = ntohl(ogds->group_id);
length = ntohs(ogds->length);
if (length < sizeof *ogds || length - sizeof *ogds > msg->size) {
@@ -5689,11 +5689,12 @@ ofputil_decode_group_desc_reply(struct
ofputil_group_desc *ogd,
return OFPERR_OFPBRC_BAD_LEN;
}
- return ofputil_pull_buckets(msg, length - sizeof *ogds, &ogd->buckets);
+ return ofputil_pull_buckets(msg, length - sizeof *ogds, &gd->buckets);
}
struct ofpbuf *
-ofputil_encode_group_mod(enum ofp_version ofp_version, const struct
ofputil_group_mod *gm)
+ofputil_encode_group_mod(enum ofp_version ofp_version,
+ const struct ofputil_group_mod *gm)
{
struct ofpbuf *b;
struct ofp11_group_mod *ogm;
@@ -5705,11 +5706,14 @@ ofputil_encode_group_mod(enum ofp_version ofp_version,
const struct ofputil_grou
switch (ofp_version) {
case OFP10_VERSION: {
if (gm->command == OFPGC11_ADD) {
- ovs_fatal(0, "add-group need argument \'-O openflow11(+)\'");
+ ovs_fatal(0, "add-group needs OpenFlow 1.1 or later "
+ "(\'-O OpenFlow11\')");
} else if (gm->command == OFPGC11_MODIFY) {
- ovs_fatal(0, "mod-group need argument \'-O openflow11(+)\'");
+ ovs_fatal(0, "mod-group needs OpenFlow 1.1 or later "
+ "(\'-O OpenFlow11\')");
} else {
- ovs_fatal(0, "del-groups need argument \'-O openflow11(+)\'");
+ ovs_fatal(0, "del-groups needs OpenFlow 1.1 or later "
+ "(\'-O OpenFlow11\')");
}
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index cfbc240..70a226c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6390,5 +6390,4 @@ const struct ofproto_class ofproto_dpif_class = {
NULL, /* group_dealloc */
NULL, /* group_modify */
NULL, /* group_get_stats */
- NULL, /* group_get_ref_cnt */
};
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index f0f980c..d37014f 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1493,8 +1493,6 @@ struct ofproto_class {
enum ofperr (*group_get_stats)(const struct ofgroup *,
struct ofputil_group_stats *);
- enum ofperr (*group_get_ref_cnt)(const struct ofgroup *,
- uint32_t *ref_cnt);
};
extern const struct ofproto_class ofproto_dpif_class;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index d15a32a..586e565 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4717,6 +4717,7 @@ append_group_stats(struct ofgroup *group, struct list
*replies)
? ofproto->ofproto_class->group_get_stats(group, &ogs)
: EOPNOTSUPP);
if (error) {
+ ogs.ref_count = UINT32_MAX;
ogs.packet_count = UINT64_MAX;
ogs.byte_count = UINT64_MAX;
ogs.n_buckets = group->n_buckets;
@@ -4724,13 +4725,6 @@ append_group_stats(struct ofgroup *group, struct list
*replies)
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;
- }
-
ogs.group_id = group->group_id;
calc_duration(group->created, now, &ogs.duration_sec, &ogs.duration_nsec);
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index ff5f31a..a044b14 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -1744,7 +1744,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
00 00 00 10 00 00 00 03 00 00 00 00 00 00 00 00 \
"], [0], [dnl
OFPST_GROUP_DESC reply (OF1.1) (xid=0x2):
-
group_id=8192,type=SELECT,bucket=weight:100,watch_port:1,actions=actions=output:1,bucket=weight:200,watch_port:2,actions=actions=output:2,bucket=weight:200,watch_port:3,actions=actions=output:3
+
group_id=8192,type=select,bucket=weight:100,watch_port:1,actions=output:1,bucket=weight:200,watch_port:2,actions=output:2,bucket=weight:200,watch_port:3,actions=output:3
])
AT_CLEANUP
@@ -2187,7 +2187,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
00 00 00 10 00 00 00 03 00 00 00 00 00 00 00 00 \
"], [0], [dnl
OFPT_GROUP_MOD (OF1.1) (xid=0x11223344):
- ADD
group_id=2271560481,type=SELECT,bucket=weight:100,watch_port:1,actions=actions=output:1,bucket=weight:200,watch_port:2,actions=actions=output:2,bucket=weight:200,watch_port:3,actions=actions=output:3
+ ADD
group_id=2271560481,type=select,bucket=weight:100,watch_port:1,actions=output:1,bucket=weight:200,watch_port:2,actions=output:2,bucket=weight:200,watch_port:3,actions=output:3
])
AT_CLEANUP
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index d282bd4..9294752 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -305,7 +305,7 @@ usage(void)
" add-group SWITCH FILE add group from FILE\n"
" mod-group SWITCH GROUP modify specific group\n"
" del-groups SWITCH [GROUP] delete matching GROUPs\n"
- " dump-group-fea SWITCH print group features\n"
+ " dump-group-features SWITCH print group features\n"
" dump-groups SWITCH print group description\n"
" dump-group-stats SWITCH [GROUP] print group statistics\n"
"\nFor OpenFlow switches and controllers:\n"
@@ -1944,7 +1944,7 @@ ofctl_dump_group_stats(int argc, char *argv[])
uint32_t group_id;
char *error;
- memset(&gm, 0, sizeof(gm));
+ memset(&gm, 0, sizeof gm);
error = parse_ofp_group_mod_str(&gm, OFPGC11_DELETE,
argc > 2 ? argv[2] : "",
@@ -3165,7 +3165,7 @@ static const struct command all_commands[] = {
{ "del-groups", 1, 2, ofctl_del_groups },
{ "dump-groups", 1, 1, ofctl_dump_group_desc },
{ "dump-group-stats", 1, 2, ofctl_dump_group_stats },
- { "dump-group-fea", 1, 1, ofctl_dump_group_features },
+ { "dump-group-features", 1, 1, ofctl_dump_group_features },
{ "help", 0, INT_MAX, ofctl_help },
/* Undocumented commands for testing. */
--
1.7.10.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
