[ovs-discuss] [PATCH 1/1] vswitchd: Add unixctl command to dump all flows, including hidden ones

[Justin Pettit]

Thanks for all the feedback.  I've integrated it and pushed.

--Justin


On Jul 29, 2009, at 4:46 PM, Ben Pfaff wrote:

> Justin Pettit <jpettit at nicira.com> writes:
>
>> Previously, the only way to query the flow table was to run "ovs- 
>> ofctl
>> dump-flows".  This returned most flows, but not those marked hidden  
>> by
>> secchan.  Hidden flows are setup by mechanisms such as in-band  
>> control,
>> since they must not be modified by users of the controller.  However,
>> when debugging problems on the switch, it is often useful to see what
>> the flow table is actually doing.  The new "bridge/dump-flows"  
>> command
>> added to ovs-appctl shows all flows being used by the OpenFlow stack.
>
>> +    ds_put_format(results, "duration=%"PRIu64"s, ",
>> +                  (time_msec() - rule->created) / 1000);
>
> time_msec() and rule->created both have type long long int so
> this should be %lld.
>
>> +    ds_put_format(results, "table_id=%d, " ,
>> +                  rule->cr.wc.wildcards ?  TABLEID_CLASSIFIER :  
>> TABLEID_HASH);
>
> Is this just to mimic the ovs-ofctl output format?  It's not too
> meaningful since the classifier doesn't really have two tables,
> it just presents the illusion of two tables through the OpenFlow
> interface.
>
>> +    ds_put_format(results, "priority=%"PRIu32", ", rule- 
>> >cr.priority);
>
> rule->cr.priority is an unsigned int should this should be %u.
>
>> +    ds_put_format(results, "n_packest=%"PRIu64", ", packet_count);
>
> n_packest should be n_packets presumably.
>
>> +    ds_put_format(results, "n_bytes=%"PRIu64", ", byte_count);
>> +    ofp_print_match(results, &match, true);
>> +    ofp_print_actions(results, (struct ofp_action_header *)rule- 
>> >actions,
>> +                      act_len);
>
> I would write &rule->actions->header here to avoid the cast.
>
>> +
>> +    if (!br->ofproto) {
>> +        unixctl_command_reply(conn, 501, "OpenFlow stack not  
>> initialized");
>> +        return;
>> +    }
>> +
>
> This check isn't needed (otherwise we'd need it in a lot of other
> places).
>
>> +    ds_init(&results);
>> +    ofproto_get_all_flows(br->ofproto, &results);
>> +
>> +    unixctl_command_reply(conn, 200, ds_cstr(&results));
>> +    ds_destroy(&results);
>> +}
>> +
>> static int
>> bridge_run_one(struct bridge *br)
>> {