[ovs-dev] [PATCH 4/4] ovs: Implement 802.1ag Connectivity Fault Management
blp at nicira.com
Mon Nov 29 18:42:39 UTC 2010
On Sat, Nov 27, 2010 at 07:04:57PM -0800, Ethan Jackson wrote:
> This commit implements a subset of the 802.1ag specification for
> Connectivity Fault Management (CFM) using Continuity Check Messages
> (CCM). When CFM is configured on an interface CCMs are broadcast
> at regular intervals to detect missing or unexpected connectivity.
Thanks for the revised version.
I still see a couple of incorrect "it's" in comments.
Can you indent this better:
fault = fault || !hmap_is_empty(&cfm->x_remote_mps)
Probably like this:
fault = (fault || !hmap_is_empty(&cfm->x_remote_mps)
cfm_run() could use a comment saying what it does and what it returns.
Please reindent this here too:
return ntohs(flow->dl_type) == ETH_TYPE_CFM &&
eth_addr_to_uint64(flow->dl_dst) == DEST_ADDR;
return (ntohs(flow->dl_type) == ETH_TYPE_CFM &&
eth_addr_to_uint64(flow->dl_dst) == DEST_ADDR);
Does "struct ccm" really need to be packed? At a glance it looks
properly aligned to me.
In iface_refresh_cfm_stats(), 'cfg' and 'cfm' are visually very similar,
would you mind renaming one of them? The HMAP_FOR_EACH invocation
should have a space: "HMAP_FOR_EACH (..."
iface_update_cfm() also has variables named 'cfg' and 'cfm'.
In general this could use a bit more background, for example at least a
sentence at the top of the tables explaining what they are good for. An
example would be a bonus (or you could add one to the end of
ovs-vsctl.8.in, which should already have an example for each existing
type of record).
"has connectivity to and MPs not" => "has connectivity to *any* MPs not"
(see ma_name) should be (see <ref column="ma_name"/>).
"ovs" should be <code>ovs</code>
The received_time column (now that I see what it is) is a little
problematic because it requires clock synchronization between
ovs-vswitchd and the database client. Is there a way to reformulate
this column in terms of an elapsed time, e.g. something like
"milliseconds since last received" or "number of milliseconds elapsed
past expected received time" or whatever?
Independently, I'm not sure that it's a good idea to expose the "current
time in milliseconds" format outside OVS. I don't think we've done that
before. Seconds since the epoch as a floating-point number might be
better, if we need a current time at all.
The descriptions don't make it entirely clear which columns the client
updates and which ones ovs-vswitchd updates. It might be a good idea to
put them into separate column groups.
More information about the dev