Ben Pfaff wrote:
> On Mon, Dec 17, 2018 at 12:24:04PM +0800, solomon wrote:
>> After creating a group with hash select type，then we need to insert a new
>> bucket with weight，
>> But，it fails. Commands are as following:
>>
>> # ovs-ofctl -O OpenFlow15 add-group br0 "group_id=10, type=select,
>> selection_method=hash,fields=tcp_src,
>> bucket=bucket_id=10,weight:99,actions=output:1,
>> bucket=bucket_id=20,weight:199,actions=output:1 "
>>
>> # ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10,type=select
>> command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
>> ovs-ofctl: type is not needed
>>
>> # ovs-ofctl -O OpenFlow15 insert-buckets br0 "group_id=10
>> command_bucket_id=last,bucket=bucket_id=3,weight=100,actions=output:1"
>> ovs-ofctl: Only select groups can have bucket weights.
>>
>>
>> This patch can help us. However, for other types that are not select, the
>> check of the parameters
>> is not strict, but it does not affect their function, because other types do
>> not use this weight parameter.
>>
>>
>> v3--v4:
>> 1. remove the redundant space in testcase。
>> ref：https://api.travis-ci.org/v3/job/467942792/log.txt
>>
>> v2-->v3:
>> 1. add testcase.
>>
>> v1-->v2:
>> 1. fix warning from 0-day robot.
>>
>>
>> Signed-off-by: solomon <[email protected]>
>
> Thanks for the revision.
>
> I think that, if we apply this, then every OFPTGC15_INSERT_BUCKET that
> ovs-ofctl sends will include a weight. That is OK if it is talking to
> the latest ovs-vswitchd, but anything older will reject it. So, I think
> that it is better if OFPTGC15_INSERT_BUCKET only includes a weight if
> the user supplied one. I think that we can do that easily, by checking
> for a nonzero weight instead of the command, and if we do that then we
> don't need the command anyway. So here is what I would suggest folding
> in:
>
I have understood what you mean. And I have send new version patch with v5.
Thanks for your review.
> diff --git a/lib/ofp-group.c b/lib/ofp-group.c
> index d9eba735da58..92ff35842986 100644
> --- a/lib/ofp-group.c
> +++ b/lib/ofp-group.c
> @@ -1177,8 +1177,7 @@ ofputil_put_ofp11_bucket(const struct ofputil_bucket
> *bucket,
> static void
> ofputil_put_ofp15_bucket(const struct ofputil_bucket *bucket,
> uint32_t bucket_id, enum ofp11_group_type
> group_type,
> - struct ofpbuf *openflow, enum ofp_version
> ofp_version,
> - int group_command)
> + struct ofpbuf *openflow, enum ofp_version
> ofp_version)
> {
> struct ofp15_bucket *ob;
> size_t start, actions_start, actions_len;
> @@ -1190,9 +1189,7 @@ ofputil_put_ofp15_bucket(const struct ofputil_bucket
> *bucket,
> ofpacts_put_openflow_actions(bucket->ofpacts, bucket->ofpacts_len,
> openflow, ofp_version);
> actions_len = openflow->size - actions_start;
> -
> - if (group_type == OFPGT11_SELECT
> - || group_command == OFPGC15_INSERT_BUCKET) {
> + if (group_type == OFPGT11_SELECT || bucket->weight) {
> ofpprop_put_u16(openflow, OFPGBPT15_WEIGHT, bucket->weight);
> }
> if (bucket->watch_port != OFPP_ANY) {
> @@ -1269,7 +1266,7 @@ ofputil_append_ofp15_group_desc_reply(const struct
> ofputil_group_desc *gds,
> start_buckets = reply->size;
> LIST_FOR_EACH (bucket, list_node, buckets) {
> ofputil_put_ofp15_bucket(bucket, bucket->bucket_id,
> - gds->type, reply, version, -1);
> + gds->type, reply, version);
> }
> ogds = ofpbuf_at_assert(reply, start_ogds, sizeof *ogds);
> ogds->type = gds->type;
> @@ -2069,8 +2066,7 @@ ofputil_encode_ofp15_group_mod(enum ofp_version
> ofp_version,
> bucket_id = bucket->bucket_id;
> }
>
> - ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b, ofp_version,
> - gm->command);
> + ofputil_put_ofp15_bucket(bucket, bucket_id, gm->type, b,
> ofp_version);
> }
> ogm = ofpbuf_at_assert(b, start_ogm, sizeof *ogm);
> ogm->command = htons(gm->command != OFPGC11_ADD_OR_MOD || group_existed
> < 0
> diff --git a/tests/ofproto.at b/tests/ofproto.at
> index 0af4fd731573..15a2914836d4 100644
> --- a/tests/ofproto.at
> +++ b/tests/ofproto.at
> @@ -679,7 +679,7 @@ AT_CHECK([ovs-ofctl -O OpenFlow11 -vwarn insert-buckets
> br0 group_id=1234,comman
> [ovs-ofctl: insert-bucket needs OpenFlow 1.5 or later ('-O OpenFlow15')
> ])
>
> -#Verify insert-buckets command to insert bucket with weight value for select
> group.
> +# Verify insert-buckets command to insert bucket with weight value for
> select group.
> AT_CHECK([ovs-ofctl -O OpenFlow15 --strict del-groups br0 group_id=1234])
> AT_DATA([groups.txt], [dnl
>
> group_id=1234,type=select,selection_method=hash,bucket=bucket_id=1,weight:100,output:11
>
--
Thanks
Solomon
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
