[ovs-dev] [PATCH 3/4] Add init/destroy funcs and handle Table Features msgs.

Alexander Wu alexander.wu at huawei.com
Fri Nov 1 13:33:23 UTC 2013


On 01/11/2013 12:32, Simon Horman wrote:
> On Sat, Oct 26, 2013 at 06:15:30PM +0800, Alexander Wu wrote:
>> Add some functions to init table features(use ofp13_* struct
>>   currently, change it to ofputil later).
>> Use the encode/decode functions to handle table features request.
>>
>> Currently we just implement GET table feature.
>> SET table feature may be a hard job, we'll do it later.
>
> I'm confused, the patchset seems to implement SET though
> not expose it to ovs-ofctl.
>
> If the SET implementation is not complete perhaps it is better
> to remove it. If it is complete perhaps it should be exposed to ovs-ofctl
> along with GET.
>
> If the SET implementation is not complete and unlikely to be complete in
> the forseeable future then it seems to me that the implementation could be
> greatly simplified by just algorithmically returning the values that are
> currently initialised by table_features_init() and not actually allocating
> the features at all in ofproto_init_tables().

It's not done because I think:
   controller/user SET table features, and then switch should
    be limited to the features, like this:

     controller says: you could only use OUTPUT,xxx,yyy action.
     switch: ok.(silently)

   Is it right? I'm not sure because I don't find the definition in spec.

Agreed to you suggesstion, I'll split the SET table-features code.

>>
>> And now the cli we implement is very crude. Maybe it could
>> display better.
>>
>> Signed-off-by: Alexander Wu <alexander.wu at huawei.com>
>> ---
>>   lib/rconn.c           |    4 +-
>>   ofproto/ofproto.c     |  384 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   utilities/ovs-ofctl.c |   17 +++
>>   3 files changed, 402 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/rconn.c b/lib/rconn.c
>> index 96b3579..c441962 100644
>> --- a/lib/rconn.c
>> +++ b/lib/rconn.c
>> @@ -1381,8 +1381,6 @@ is_admitted_msg(const struct ofpbuf *b)
>>       case OFPTYPE_GROUP_DESC_STATS_REPLY:
>>       case OFPTYPE_GROUP_FEATURES_STATS_REQUEST:
>>       case OFPTYPE_GROUP_FEATURES_STATS_REPLY:
>> -    case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
>> -    case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
>>           return false;
>>
>>       case OFPTYPE_PACKET_IN:
>> @@ -1429,6 +1427,8 @@ is_admitted_msg(const struct ofpbuf *b)
>>       case OFPTYPE_FLOW_MONITOR_CANCEL:
>>       case OFPTYPE_FLOW_MONITOR_PAUSED:
>>       case OFPTYPE_FLOW_MONITOR_RESUMED:
>> +    case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
>> +    case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
>>       default:
>>           return true;
>>       }
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 91a39ef..03f6e14 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -559,6 +559,45 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
>>       return 0;
>>   }
>>
>> +static void
>> +table_features_init(struct ofputil_table_features *tf, int table_id);
>
> Is it possible to avoid this forward declaration by re-ordering the
> functions?
>

Possible, I'll rethink it, thanks.

>> +
>> +static void
>> +ofproto_init_max_table_features(struct ofproto *ofproto)
>> +{
>> +    int i;
>> +    struct ofputil_table_features *tf;
>> +
>> +    for (i = 0; i < ofproto->n_tables; i++) {
>> +        tf = &ofproto->otf[i];
>> +        memset(tf, 0, sizeof(*tf));
>> +        table_features_init(tf, i);
>> +    }
>> +}
>> +
>> +static void
>> +ofproto_init_real_table_features(struct ofproto *ofproto)
>> +{
>> +    int i;
>> +    struct ofputil_table_features *tf;
>> +
>> +    for (i = 0; i < ofproto->n_tables; i++) {
>> +        tf = &ofproto->tables[i].tf;
>> +        memset(tf, 0, sizeof(*tf));
>> +        table_features_init(tf, i);
>> +    }
>> +}
>
> It seems to me that you could refactor ofproto_init_max_table_features()
> and ofproto_init_real_table_features() into a single function paramatised
> over the array of ofputil_table_features to use.
>

Yes, I do this before, but I think split them could improve the performance.
The ofproto->otf[0-255] are compact.

>> +
>> +static void
>> +ofproto_init_table_features(struct ofproto *ofproto)
>> +{
>> +    /* init the max table features for get */
>> +    ofproto_init_max_table_features(ofproto);
>> +
>> +    /* set the real one */
>> +    ofproto_init_real_table_features(ofproto);
>> +}
>> +
>>   /* Must be called (only) by an ofproto implementation in its constructor
>>    * function.  See the large comment on 'construct' in struct ofproto_class for
>>    * details. */
>> @@ -575,8 +614,274 @@ ofproto_init_tables(struct ofproto *ofproto, int n_tables)
>>       OFPROTO_FOR_EACH_TABLE (table, ofproto) {
>>           oftable_init(table);
>>       }
>> +    ofproto_init_table_features(ofproto);
>> +}
>> +
>> +static void
>> +table_instructions_init(struct ofputil_table_features *tf,
>> +                        enum ofputil_table_feature_prop_type type)
>> +{
>> +    const int OFPIT13_END = OFPIT13_METER;
>
> Can you just use OFPIT13_METER directly?
>
> If not, I don't think its necessary or in keeping with the coding style
> to make it const or capitalise the variable name.
>

Maybe OFPIT13_END is more meaningful?
I'll rethink it, thanks.

>> +    int i = 0;
>> +    int olen = 0;
>> +
>> +    struct ofputil_instruction *inst = NULL;
>> +    int element_size = sizeof(struct ofputil_instruction);
>> +
>> +    olen = OFPIT13_END * element_size;
>> +
>> +    inst = (struct ofputil_instruction *)xmalloc(olen);
>> +    memset(inst, 0, olen);
>> +
>> +    tf->props[type].data = (uint8_t*)inst;
>> +    for(i = 0 ; i < OFPIT13_END ; i++){
>> +        /* ofp11(3)_instruction_type */
>> +        inst[i].type = i + 1;
>> +        inst[i].len = element_size;
>> +    }
>> +
>> +    tf->props[type].type = type;
>> +    tf->props[type].length = olen + sizeof(struct ofp13_table_feature_prop_header);
>
> This line is greater than 80 columns wide. I'm unsure if it matters.
>

You're right, it's greater than 80. I'll fix it.

>> +
>> +    tf->n_property++;
>> +}
>> +
>> +static void
>> +table_features_INSTRUCTIONS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_instructions_init(tf, OFPUTIL_INSTRUCTIONS);
>> +}
>> +
>> +static void
>> +table_features_INSTRUCTIONS_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_instructions_init(tf, OFPUTIL_INSTRUCTIONS_MISS);
>> +
>> +}
>
> I'm unsure that there is much value in these wrapper functions.
> Are they use as callbacks or as pointers in a function table?
> If not I suggest just open-coding the one-line that they encapsulate.
>
> If you really need these, and the numerous similar ones below,
> then perhaps they could at all be grouped together. And automatically
> generated using a macro.
>

I think the function's name is meaningful,
and it's comfy to custom the functions for each table features in the future.
How about make them 'inline'?

I'm planing to replace the numerous functions to a macro,
thanks!

>> +
>> +static void
>> +table_next_table_init(struct ofputil_table_features *tf,enum ofputil_table_feature_prop_type type)
>
> I believe that the following is more in keeping with the prevailing coding
> style:
>
> static void
> table_next_table_init(struct ofputil_table_features *tf,
>                        enum ofputil_table_feature_prop_type type)
>
>

Yes, I'll fix it, thanks.
I think I should write a shell script to detect these problems.

>> +{
>> +    const int MAX_TABLE = OFTABLE_NUM;
>
> See comments regarding OFPIT13_END above.
>

I'll fix it, thanks.

>> +
>> +    int olen = 0;
>
> olen is re-initialised immediately below.
> I would either initialise it here to the value set below or
> not initialise it at all here.
>

Yes, I'll fix the unnecessary initialization, thanks.

>> +    uint8_t i = 0;
>> +    uint8_t cursor = 0;
>> +    uint8_t *next_table = NULL;
>> +
>> +    olen = (MAX_TABLE - tf->table_id - 1) * sizeof(uint8_t);
>
>> +
>> +    /* last table */
>> +    if (0 == olen)
>> +        return ;
>
> The following would be more in keeping with the prevailing coding style.
>
>         if (olen == 0) {
>             return;
>         }
>

I'll replace it, thanks.

>> +
>> +    next_table = xmalloc(olen);
>> +    memset(next_table, 0, olen);
>
> Does next_table really need to be zeroed?
> If so you can replace the above two lines with:
>
>          next_table = xzalloc(olen);
>
>

Thanks, I'll fix it.

>> +
>> +    tf->props[type].data = next_table;
>> +
>> +    for(i = tf->table_id; i < MAX_TABLE - 1; i++)
>> +        next_table[cursor++] = i + 1;
>
> Please add a space after for. And use { }
>
>         for (i = tf->table_id; i < MAX_TABLE - 1; i++) {
>             next_table[cursor++] = i + 1;
>         }
>

Thanks, I'll fix it.
Is it prevailing coding style?

>> +
>> +    tf->props[type].type = OFPUTIL_NEXT_TABLES;
>> +    tf->props[type].length = olen + sizeof(struct ofp13_table_feature_prop_header);
>
> The line above is more than 80 columns wide.
>

Thanks, I'll fix it.

>> +    tf->n_property++;
>> +}
>> +
>> +static void
>> +table_features_NEXT_TABLES_init(struct ofputil_table_features *tf)
>> +{
>> +    table_next_table_init(tf, OFPUTIL_NEXT_TABLES);
>> +}
>> +static void
>> +table_features_NEXT_TABLES_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_next_table_init(tf, OFPUTIL_NEXT_TABLES_MISS);
>> +}
>
> See comment regarding table_features_INSTRUCTIONS_MISS_init() and
> table_features_INSTRUCTIONS_init().
>

Thanks, I'll update the functions.

>> +
>> +static void
>> +table_action_init(struct ofputil_table_features *tf,
>> +                enum ofputil_table_feature_prop_type type)
>> +{
>> +    int i = 0 ,olen = 0;
>> +    struct ofp_action_header *action = NULL;
>> +    const int ACTION_NUM = OFPAT13_POP_PBB + 1;
>> +
>> +    olen = ACTION_NUM * sizeof(struct ofp_action_header);
>> +
>> +    action = (struct ofp_action_header *)xzalloc(olen);
>> +
>> +    tf->props[type].data = (uint8_t*)action;
>> +
>> +    for(i = 0 ; i < ACTION_NUM; i++){
>
> Please add a space before '(' and after ')'
>
>         for (i = 0 ; i < ACTION_NUM; i++) {
>

Right.
I believe I should write a script to detect the CodingStyle problems.

>> +           action[i].type = i;
>
> The indentation of the above line is inconsistent with the one below.
>

I'll fix it, thanks.

>> +        action[i].len = 8;
>> +    }
>> +
>> +    tf->props[type].length = olen + sizeof(struct ofp13_table_feature_prop_header);
>
> The line above is more than 80 columns wide.
>

I'll fix it, thanks.

>> +    tf->props[type].type = type;
>> +       tf->n_property++;
>>   }
>>
>> +
>> +static void
>> +table_features_WRITE_ACTIONS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_action_init(tf, OFPUTIL_WRITE_ACTIONS);
>> +}
>> +
>> +static void
>> +table_features_WRITE_ACTIONS_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_action_init(tf, OFPUTIL_WRITE_ACTIONS_MISS);
>> +}
>> +
>> +static void
>> +table_features_APPLY_ACTIONS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_action_init(tf, OFPUTIL_APPLY_ACTIONS);
>> +}
>> +
>> +static void
>> +table_features_APPLY_ACTIONS_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_action_init(tf, OFPUTIL_APPLY_ACTIONS_MISS);
>> +}
>
> See comment regarding table_features_INSTRUCTIONS_MISS_init() and
> table_features_INSTRUCTIONS_init().
>

Thanks, I'll update the functions.

>> +
>> +static void
>> +table_oxm_init(struct ofputil_table_features *tf,
>> +                enum ofputil_table_feature_prop_type type)
>
> I think there is an extra space before enum, it should be aligned
> to start in the column after the '(' in the line above.
>
> static void
> table_oxm_init(struct ofputil_table_features *tf,
>                 enum ofputil_table_feature_prop_type type)
>

Yes. I'll fix it.

>> +{
>> +    int i = 0 ,olen = 0;
>
> The space should come after the , not before it.
> But I seem to recall that when initialising variable in their declaration
> Ben prefers them on individual lines.
>
> Also olen is set to a different value immediately below.
>
>         int i = 0;
>         int olen = 0;
>
> Or re-arnage things so you can set olen to its final value.
>
>

It's unnecessary to init some variables.
I'll fix it, thanks.

>> +    const int OXM_NUM = get_oxm_num();
>
> I don't think this should const or that the variable name should
> not be capitalised.
>
>> +    uint32_t *oxm_ids = NULL;
>> +
>> +    olen = sizeof(uint32_t) * OXM_NUM;
>> +    oxm_ids = xzalloc(olen);
>> +
>> +    tf->props[type].data = (uint8_t*)oxm_ids;
>> +
>> +    for (i = 0; i < OXM_NUM; i++) {
>> +        oxm_ids[i] = oxm_variables[i].data;
>> +    }
>> +
>> +    tf->props[type].length = olen + sizeof(struct ofp13_table_feature_prop_header);
>
> The line above is more than 80 columns wide.
>

Yes, I'll fix it later, thanks.

>> +    tf->props[type].type = type;
>> +    tf->n_property++;
>> +}
>> +
>> +static void
>> +table_features_MATCH_init(struct ofputil_table_features *tf)
>> +{
>> +    table_oxm_init(tf, OFPUTIL_MATCH);
>> +    return ;
>> +}
>> +
>> +
>> +static void
>> +table_features_WILDCARDS_init(struct ofputil_table_features *tf)
>> +{
>> +     table_oxm_init(tf, OFPUTIL_WILDCARDS);
>> +     return;
>> +}
>> +
>> +static void
>> +table_features_WRITE_SETFIELD_init(struct ofputil_table_features *tf)
>> +{
>> +    table_oxm_init(tf, OFPUTIL_WRITE_SETFIELD);
>> +    return ;
>> +}
>> +
>> +static void
>> +table_features_WRITE_SETFIELD_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_oxm_init(tf, OFPUTIL_WRITE_SETFIELD_MISS);
>> +    return ;
>> +}
>> +
>> +static void
>> +table_features_APPLY_SETFIELD_init(struct ofputil_table_features *tf)
>> +{
>> +    table_oxm_init(tf, OFPUTIL_APPLY_SETFIELD);
>> +    return ;
>> +}
>> +
>> +static void
>> +table_features_APPLY_SETFIELD_MISS_init(struct ofputil_table_features *tf)
>> +{
>> +    table_oxm_init(tf, OFPUTIL_APPLY_SETFIELD_MISS);
>> +    return;
>> +}
>> +
>> +static void
>> +table_features_EXPERIMENTER_init(
>> +    struct ofputil_table_features *tf OVS_UNUSED)
>> +{
>> +    return;
>> +}
>> +
>> +static void
>> +table_features_EXPERIMENTER_MISS_init(
>> +    struct ofputil_table_features *tf OVS_UNUSED)
>> +{
>> +    return;
>> +}
>
> See comment regarding table_features_INSTRUCTIONS_MISS_init() and
> table_features_INSTRUCTIONS_init().
>

Thanks, I'll fix it.

>> +
>> +static void
>> +table_features_init(struct ofputil_table_features *tf, int table_id)
>> +{
>> +    tf->table_id = table_id;
>> +    snprintf(tf->name, OFP_MAX_TABLE_NAME_LEN, "%s%d", "table", table_id);
>> +
>> +    tf->metadata_match = OVS_BE64_MAX;
>> +    tf->metadata_write = OVS_BE64_MAX;
>
> metadata_match and metadata_write are host endian (uint64_t) but
> here you are initialising them as big endian (OVS_BE64_MAX). Either
> their type of their initialisation is incorrect.
>

Yes, it should be a macro like UINT64_MAX,
I'll fix it.

>> +    tf->config = OFPTC11_TABLE_MISS_MASK;
>> +    tf->max_entries = 1000000;
>
> I'm curuious, why 1000000?
> If it is an abitrary limit I would add a comment saying so.
> Also, I would be tempted to make the limit the maximum possible value.
> I.e. UINT32_MAX
>

I write this value according to handle_table_stats_request.
Should I replace it with a macro?

Long time ago(1.7) I see there is a actual limit while processing flow table.
The limit seems like to be 1000000, but I can't find it now.

>> +
>> +    tf->length = 0;        /* useless now */
>
> Why is it useless?
>

I'll update it at last.

>> +    tf->n_property = 0;    /* update when needed */
>> +
>> +    /* CHECK the props are inited to 0. */
>> +    memset(tf->props, 0, sizeof(tf->props));
>> +
>> +    /* NOTE now the implement use ofp13_*, change it to ofputil_* later. */
>> +    table_features_INSTRUCTIONS_init(tf);
>> +    table_features_INSTRUCTIONS_MISS_init(tf);
>> +    table_features_NEXT_TABLES_init(tf);
>> +    table_features_NEXT_TABLES_MISS_init(tf);
>> +    table_features_WRITE_ACTIONS_init(tf);
>> +    table_features_WRITE_ACTIONS_MISS_init(tf);
>> +    table_features_APPLY_ACTIONS_init(tf);
>> +    table_features_APPLY_ACTIONS_MISS_init(tf);
>> +    table_features_MATCH_init(tf);
>> +    table_features_WILDCARDS_init(tf);
>> +    table_features_WRITE_SETFIELD_init(tf);
>> +    table_features_WRITE_SETFIELD_MISS_init(tf);
>> +    table_features_APPLY_SETFIELD_init(tf);
>> +    table_features_APPLY_SETFIELD_MISS_init(tf);
>> +    table_features_EXPERIMENTER_init(tf);
>> +    table_features_EXPERIMENTER_MISS_init(tf);
>> +
>> +    /* FIXME Now we search the array at last. */
>
> Is this FIXME still valid? Is the code usable or at least harmless as-is?
>

Yes. I should remove this n_property or the n_property in init functions.
Or they should be confusing.

>> +    tf->n_property = 0xff;
>> +}
>> +
>> +static void
>> +table_features_destroy(struct ofputil_table_features *tf)
>> +{
>> +    int i = 0;
>> +    int n = tf->n_property;
>> +    struct ofputil_table_feature_prop_header *props = tf->props;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        if (props[i].data) {
>> +            free(props[i].data);
>> +            props[i].data = NULL;
>> +        }
>> +    }
>> +}
>> +
>> +
>>   /* To be optionally called (only) by an ofproto implementation in its
>>    * constructor function.  See the large comment on 'construct' in struct
>>    * ofproto_class for details.
>> @@ -1216,6 +1521,7 @@ static void
>>   ofproto_destroy__(struct ofproto *ofproto)
>>       OVS_EXCLUDED(ofproto_mutex)
>>   {
>> +    int i;
>>       struct oftable *table;
>>
>>       ovs_assert(list_is_empty(&ofproto->pending));
>> @@ -1242,6 +1548,10 @@ ofproto_destroy__(struct ofproto *ofproto)
>>       shash_destroy(&ofproto->port_by_name);
>>       simap_destroy(&ofproto->ofp_requests);
>>
>> +    for (i = 0; i < ofproto->n_tables; i++) {
>> +        table_features_destroy(&ofproto->otf[i]);
>> +    }
>> +
>>       OFPROTO_FOR_EACH_TABLE (table, ofproto) {
>>           oftable_destroy(table);
>>       }
>> @@ -5648,6 +5958,74 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
>>       }
>>   }
>>
>> +static void table_features_get_by_id(struct ofproto *ofproto,
>> +            int id, struct ofputil_table_features *features)
>
> static void
> table_features_get_by_id(struct ofproto *ofproto, int id,
>                           struct ofputil_table_features *features)
>

Thanks, I'll fix it.

>> +{
>> +    /* CHECK if it doesn't need copy */
>
> Does this comment still apply?
>

In OFPMP_TABLE_FEATURES get case, it gets ofproto->otf, and it doesn't need copy,
although it has been copied now, I'll fix it later, thanks.

>> +    memcpy(features, &ofproto->tables[id].tf, sizeof(*features));
>
> I think "sizeof *features" is preferable to sizeof(*features)
> as sizeof is not a function or a macro.
>

Yes, it's a operator. I'll fix it.

>> +}
>> +
>> +
>> +static void table_features_set(struct ofproto *ofproto,
>> +                            struct ofputil_table_features *features)
>
> static void
> table_features_set(struct ofproto *ofproto,
>                     struct ofputil_table_features *features)
>

I'll fix it, thanks.

>> +{
>> +    uint8_t table_id = features->table_id;
>> +    struct ofputil_table_features *tf = &ofproto->tables[table_id].tf;
>> +
>> +    /*
>> +     * NOTE the props' pointers have been copied, so we free the olds
>> +     * Currently we use table_features_destroy func, FIXME with a specified
>> +     */
>
> Does this comment still apply?
>

Yes.
The function works fine and I'll fix it later, thanks.

>> +    table_features_destroy(tf);
>> +    memcpy(tf, features, sizeof(*features));
>> +
>> +    return ;
>> +}
>> +
>> +
>
> One blank line should be sufficient here.
>

Thanks, I'll fix it.

>> +static enum ofperr
>> +handle_table_features_stats_request(struct ofconn *ofconn,
>> +                                    const struct ofp_header *oh)
>> +{
>> +
>> +    struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
>> +    struct ofputil_table_features request_features[ofproto->n_tables];
>> +    struct ofputil_table_features reply_features[ofproto->n_tables];
>> +    int features_num;
>> +    uint32_t request_flag = 0;
>> +    int error;
>> +    int i;
>> +
>> +    struct list replies;
>> +
>> +    /* decode request */
>> +    error = ofputil_decode_table_features_request(oh, &features_num, request_features,
>
> The line above is greater than 80 columns wide.
>

Thanks, I'll fix it.

>> +                                            &request_flag);
>> +    if (error) {
>> +        return error;
>> +    }
>> +
>> +    switch (request_flag) {
>> +    case OTF_GET: {
>> +        ofpmp_init(&replies, oh);
>> +        for (i = 0; i < ofproto->n_tables; i++) {
>> +            table_features_get_by_id(ofproto, i, &reply_features[0]);
>> +            ofputil_append_table_features_reply(reply_features, &replies);
>> +        }
>> +        ofconn_send_replies(ofconn, &replies);
>> +        break;
>> +    }
>> +    case OTF_SET: {
>> +        for (i = 0; i < features_num; i++) {
>> +            table_features_set(ofproto, &request_features[i]);
>> +        }
>> +        break;
>> +    }
>
> There is no need for the outer-most { } in the OTF_SET case unless you
> wish to declare i there and in the OTF_GET case.
>

Yes, it's just a defensive method, I'll remove the { } if necessary.

>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static enum ofperr
>>   handle_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
>>   {
>> @@ -5680,6 +6058,7 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
>>       if (error) {
>>           return error;
>>       }
>> +
>
> This change seems unrelated to the rest of the patch.
> Please remove it.
>

I'll remove it, thanks.

>>       if (oh->version >= OFP13_VERSION && ofpmsg_is_stat_request(oh)
>>           && ofpmp_more(oh)) {
>>           /* We have no buffer implementation for multipart requests.
>> @@ -5797,9 +6176,11 @@ handle_openflow__(struct ofconn *ofconn, const struct ofpbuf *msg)
>>       case OFPTYPE_GROUP_FEATURES_STATS_REQUEST:
>>           return handle_group_features_stats_request(ofconn, oh);
>>
>> +    case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
>> +        return handle_table_features_stats_request(ofconn, oh);
>> +
>>           /* FIXME: Change the following once they are implemented: */
>>       case OFPTYPE_QUEUE_GET_CONFIG_REQUEST:
>> -    case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
>>           /* fallthrough */
>>
>>       case OFPTYPE_HELLO:
>> @@ -6512,6 +6893,7 @@ oftable_destroy(struct oftable *table)
>>       oftable_disable_eviction(table);
>>       classifier_destroy(&table->cls);
>>       free(table->name);
>> +    table_features_destroy(&table->tf);
>>   }
>>
>>   /* Changes the name of 'table' to 'name'.  If 'name' is NULL or the empty
>
> Its not a big deal. But it seems to me that the ovs-ofctl.c changes
> below would comfortably go into a separate patch to be applied after this
> one.
>

Yes. I'll split the patch into separated ones.

>> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
>> index 1a1d423..5970731 100644
>> --- a/utilities/ovs-ofctl.c
>> +++ b/utilities/ovs-ofctl.c
>> @@ -281,6 +281,7 @@ usage(void)
>>              "  show SWITCH                 show OpenFlow information\n"
>>              "  dump-desc SWITCH            print switch description\n"
>>              "  dump-tables SWITCH          print table stats\n"
>> +           "  dump-table-features SWITCH  print table features\n"
>>              "  mod-port SWITCH IFACE ACT   modify port behavior\n"
>>              "  mod-table SWITCH MOD        modify flow table behavior\n"
>>              "  get-frags SWITCH            print fragment handling behavior\n"
>> @@ -649,6 +650,21 @@ ofctl_dump_tables(int argc OVS_UNUSED, char *argv[])
>>       dump_trivial_stats_transaction(argv[1], OFPRAW_OFPST_TABLE_REQUEST);
>>   }
>>
>> +static void
>> +ofctl_dump_table_features(int argc OVS_UNUSED, char *argv[])
>> +{
>> +    struct ofpbuf *request;
>> +    struct vconn *vconn;
>> +
>> +    open_vconn(argv[1], &vconn);
>
> Do you need to check argc to make sure accessing argv[1] is valid?
>

I think this line guarantee argv[1] is valid.
+    { "dump-table-features", 1, 1, ofctl_dump_table_features },

>> +    request = ofputil_encode_table_features_request(vconn_get_version(vconn));
>> +    if (request) {
>> +        dump_stats_transaction(vconn, request);
>> +    }
>> +
>> +    vconn_close(vconn);
>> +}
>> +
>>   static bool
>>   fetch_port_by_features(const char *vconn_name,
>>                          const char *port_name, ofp_port_t port_no,
>> @@ -3323,6 +3339,7 @@ static const struct command all_commands[] = {
>>       { "snoop", 1, 1, ofctl_snoop },
>>       { "dump-desc", 1, 1, ofctl_dump_desc },
>>       { "dump-tables", 1, 1, ofctl_dump_tables },
>> +    { "dump-table-features", 1, 1, ofctl_dump_table_features },
>>       { "dump-flows", 1, 2, ofctl_dump_flows },
>>       { "dump-aggregate", 1, 2, ofctl_dump_aggregate },
>>       { "queue-stats", 1, 3, ofctl_queue_stats },
>> --
>> 1.7.3.1.msysgit.0
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>>
>
> .
>


-- 
Best Regards
Alexander Wu




More information about the dev mailing list