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

Jesse Gross jesse at nicira.com
Wed Dec 22 23:28:41 UTC 2010


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.

> +{
> +    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?

> +        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().

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.

> +
> +        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.

> +        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.




More information about the dev mailing list