[ovs-dev] [PATCH 2/6] ovs-dpctl: Print extended information about vports.

Justin Pettit jpettit at nicira.com
Thu Dec 23 02:02:21 UTC 2010


On Dec 22, 2010, at 3:28 PM, Jesse Gross wrote:

> On Wed, Dec 22, 2010 at 3:04 AM, Justin Pettit <jpettit at nicira.com> wrote:
>> +void
>> +format_odp_port_type(struct ds *ds, const struct odp_port *p)
> 
> The convention that we have usually followed is input arguments
> followed by output arguments.

Yes, but I followed the convention for the other similar functions in that file (and ofp-print.c).

>> +{
>> +    if (!strcmp(p->type, "gre")
>> +            || !strcmp(p->type, "capwap")) {
>> +        struct tnl_port_config config;
>> +
>> +        memcpy(&config, p->config, sizeof config);
> 
> What's the reason for the memcpy?  Wouldn't a cast work just as well?

Yeah, I think that was due to some intermediate thing I did and never reverted.  Thanks.

>> +        ds_put_format(ds, " (%s: remote_ip="IP_FMT,
>> +                p->type, IP_ARGS(&config.daddr));
>> +
>> +        if (config.saddr) {
>> +            ds_put_format(ds, ", local_ip="IP_FMT, IP_ARGS(&config.saddr));
>> +        }
>> +
>> +        if (config.in_key) {
>> +            ds_put_format(ds, ", in_key=%"PRIu64, ntohll(config.in_key));
>> +        }
> 
> In other places we print the key in hex.  Actually, this should just
> follow the same format as format_odp_flow_key().

Thanks.  Updated.

> This is just a subset of the configuration parameters that can be set
> on a port.  What was the rational for the choice of these?  It looks
> like matching criteria but I'm not sure why that's of particular
> interest in this case.

I was printing only the things that determined the uniqueness of the port.  By printing everything, it increases the likelihood of wrapping lines and making the output harder to grok.  If people feel that it would be particularly useful, it's easy enough to add.

>> +        ds_put_cstr(ds, ")");
>> +    } else if (!strcmp(p->type, "patch")) {
>> +        char peer[VPORT_CONFIG_SIZE];
>> +
>> +        ovs_strlcpy(peer, (void *)p->config, sizeof peer);
> 
> Same question here about the need to copy.

Yep.  Done.

>> +        ds_put_format(ds, " (%s: peer=%s)", p->type, peer);
>> +    } else if (strcmp(p->type, "system")) {
>> +        ds_put_format(ds, " (%s)", p->type);
>> +    }
>> +}
> 
> I think this is nice functionality.  However, it strikes me that since
> netdevs are modular it would be better if we had a format function for
> netdevs instead of centralizing it.  Then the format code could be
> next to the parsing code, which will probably reduce the likelihood of
> differences and drift over time.

Good idea.  I can think of a few ways to do it at various levels of abstraction.  Let's talk about this, and I'd be happy to add it in a subsequent patch.

--Justin






More information about the dev mailing list