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

Jesse Gross jesse at nicira.com
Thu Dec 23 09:42:44 UTC 2010


On Wed, Dec 22, 2010 at 9:02 PM, Justin Pettit <jpettit at nicira.com> wrote:
> 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:
>>> +{
>>> +    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.

I thought about this a little more and you might need the copy due to
strict aliasing rules.  The type of p->config is u64 and a pointer to
that is not compatible with struct tnl_port_config *.  In practice,
this is highly unlikely to cause a problem due to the access pattern
but it's not quite standards conformant.

The patch port peer name should be OK, since casts to char * are
specifically allowed.

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

Yeah, I'm just not sure why the uniqueness characteristics are
particularly interesting.  Since it's really a debugging tool, more
information is more useful - if everything were working fine, the name
would be sufficient as that is also unique.  What if we put each field
on a new line (probably indented)?  If we only print the non-default
fields it shouldn't get too long.

Just printing the fields needed for uniqueness is also somewhat
inconsistent with patch ports, since the peer does not need to be
unique.

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

Hmm, when I think about this more it's a little messier that I
thought.  Originally, I was thinking that we could just have a static
function (in the C++ sense) that takes the type and config and the
netdev library would hand it off to the right netdev type.  However,
this is really vport specific.  So then the next problem is that
vports are really Linux specific and this is platform-independent
code.

We really need to separate our layers a little more cleanly since the
Linux specific communication with the kernel module bleeds over into
the platform independent pieces in a number of areas.  The Netlink
stuff will help some by explicitly using Linux conventions, which
should make it more obvious where we are using it elsewhere.  However,
there is still more to do.




More information about the dev mailing list