Currently, ofproto_port_get_bfd/cfm_status() is used to check the
bfd/cfm status change and query the status change. Users decide
what to do with the filled status struct based on the return value
of the funciton. Such design is confusing and makes the caller
code hard to read.
This commit breaks the function into a status change check function
and a status query function, so that they become easier to read and
use.
Signed-off-by: Alex Wang <al...@nicira.com>
---
ofproto/ofproto-dpif.c | 41 ++++++++++++++++++++++++-----------------
ofproto/ofproto-provider.h | 25 +++++++++++++------------
ofproto/ofproto.c | 32 +++++++++++++++++++++++++++-----
ofproto/ofproto.h | 2 ++
vswitchd/bridge.c | 25 ++++++++++++++-----------
5 files changed, 80 insertions(+), 45 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 06be234..8a4205a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -73,9 +73,6 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif);
COVERAGE_DEFINE(ofproto_dpif_expired);
COVERAGE_DEFINE(packet_in_overflow);
-/* No bfd/cfm status change. */
-#define NO_STATUS_CHANGE -1
-
struct flow_miss;
struct rule_dpif {
@@ -1819,6 +1816,14 @@ out:
return error;
}
+static bool
+cfm_status_changed(struct ofport *ofport_)
+{
+ struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+
+ return ofport->cfm ? cfm_check_status_change(ofport->cfm) : true;
+}
+
static int
get_cfm_status(const struct ofport *ofport_,
struct ofproto_cfm_status *status)
@@ -1827,15 +1832,11 @@ get_cfm_status(const struct ofport *ofport_,
int ret = 0;
if (ofport->cfm) {
- if (cfm_check_status_change(ofport->cfm)) {
- status->faults = cfm_get_fault(ofport->cfm);
- status->flap_count = cfm_get_flap_count(ofport->cfm);
- status->remote_opstate = cfm_get_opup(ofport->cfm);
- status->health = cfm_get_health(ofport->cfm);
- cfm_get_remote_mpids(ofport->cfm, &status->rmps, &status->n_rmps);
- } else {
- ret = NO_STATUS_CHANGE;
- }
+ status->faults = cfm_get_fault(ofport->cfm);
+ status->flap_count = cfm_get_flap_count(ofport->cfm);
+ status->remote_opstate = cfm_get_opup(ofport->cfm);
+ status->health = cfm_get_health(ofport->cfm);
+ cfm_get_remote_mpids(ofport->cfm, &status->rmps, &status->n_rmps);
} else {
ret = ENOENT;
}
@@ -1861,6 +1862,14 @@ set_bfd(struct ofport *ofport_, const struct smap *cfg)
return 0;
}
+static bool
+bfd_status_changed(struct ofport *ofport_)
+{
+ struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+
+ return ofport->bfd ? bfd_check_status_change(ofport->bfd) : true;
+}
+
static int
get_bfd_status(struct ofport *ofport_, struct smap *smap)
{
@@ -1868,11 +1877,7 @@ get_bfd_status(struct ofport *ofport_, struct smap *smap)
int ret = 0;
if (ofport->bfd) {
- if (bfd_check_status_change(ofport->bfd)) {
- bfd_get_status(ofport->bfd, smap);
- } else {
- ret = NO_STATUS_CHANGE;
- }
+ bfd_get_status(ofport->bfd, smap);
} else {
ret = ENOENT;
}
@@ -4922,8 +4927,10 @@ const struct ofproto_class ofproto_dpif_class = {
set_sflow,
set_ipfix,
set_cfm,
+ cfm_status_changed,
get_cfm_status,
set_bfd,
+ bfd_status_changed,
get_bfd_status,
set_stp,
get_stp_status,
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index ff539b9..0c12916 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1442,13 +1442,13 @@ struct ofproto_class {
* support CFM, as does a null pointer. */
int (*set_cfm)(struct ofport *ofport, const struct cfm_settings *s);
- /* Checks the status of CFM configured on 'ofport'. Returns 0 if the
- * port's CFM status was successfully stored into '*status'. Returns
- * negative number if there is no status change since last update.
- * Returns positive errno otherwise.
- *
- * EOPNOTSUPP as a return value indicates that this ofproto_class does not
- * support CFM, as does a null pointer.
+ /* Checks the status change of CFM on 'ofport'. Returns true if there
+ * is status change since last call or CFM is not specified. */
+ bool (*cfm_status_changed)(struct ofport *ofport);
+
+ /* Populates 'smap' with the status of CFM on 'ofport'. Returns 0 on
+ * success, or a positive errno. EOPNOTSUPP as a return value indicates
+ * that this ofproto_class does not support CFM, as does a null pointer.
*
* The caller must provide and own '*status', and it must free the array
* returned in 'status->rmps'. '*status' is indeterminate if the return
@@ -1466,12 +1466,13 @@ struct ofproto_class {
* support BFD, as does a null pointer. */
int (*set_bfd)(struct ofport *ofport, const struct smap *cfg);
+ /* Checks the status change of BFD on 'ofport'. Returns true if there
+ * is status change since last call or BFD is not specified. */
+ bool (*bfd_status_changed)(struct ofport *ofport);
+
/* Populates 'smap' with the status of BFD on 'ofport'. Returns 0 on
- * success. Returns a negative number if there is no status change since
- * last update. Returns a positive errno otherwise.
- *
- * EOPNOTSUPP as a return value indicates that this ofproto_class does not
- * support BFD, as does a null pointer. */
+ * success, or a positive errno. EOPNOTSUPP as a return value indicates
+ * that this ofproto_class does not support BFD, as does a null pointer. */
int (*get_bfd_status)(struct ofport *ofport, struct smap *smap);
/* Configures spanning tree protocol (STP) on 'ofproto' using the
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 24a709b..e0007f6 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1020,10 +1020,21 @@ ofproto_port_set_bfd(struct ofproto *ofproto,
ofp_port_t ofp_port,
}
}
+/* Checks the status change of BFD on 'ofport'.
+ *
+ * Returns true if 'ofproto_class' does not support 'bfd_status_changed'. */
+bool
+ofproto_port_bfd_status_changed(struct ofproto *ofproto, ofp_port_t ofp_port)
+{
+ struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
+ return (ofport && ofproto->ofproto_class->bfd_status_changed
+ ? ofproto->ofproto_class->bfd_status_changed(ofport)
+ : true);
+}
+
/* Populates 'status' with the status of BFD on 'ofport'. Returns 0 on
- * success. Returns a negative number if there is no status change since
- * last update. Returns a positive errno otherwise. Has no effect if
- * 'ofp_port' is not an OpenFlow port in 'ofproto'.
+ * success. Returns a positive errno otherwise. Has no effect if 'ofp_port'
+ * is not an OpenFlow port in 'ofproto'.
*
* The caller must provide and own '*status'. */
int
@@ -3738,11 +3749,22 @@ ofproto_get_netflow_ids(const struct ofproto *ofproto,
ofproto->ofproto_class->get_netflow_ids(ofproto, engine_type, engine_id);
}
+/* Checks the status change of CFM on 'ofport'.
+ *
+ * Returns true if 'ofproto_class' does not support 'cfm_status_changed'. */
+bool
+ofproto_port_cfm_status_changed(struct ofproto *ofproto, ofp_port_t ofp_port)
+{
+ struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
+ return (ofport && ofproto->ofproto_class->cfm_status_changed
+ ? ofproto->ofproto_class->cfm_status_changed(ofport)
+ : true);
+}
+
/* Checks the status of CFM configured on 'ofp_port' within 'ofproto'.
* Returns 0 if the port's CFM status was successfully stored into
* '*status'. Returns positive errno if the port did not have CFM
- * configured. Returns negative number if there is no status change
- * since last update.
+ * configured.
*
* The caller must provide and own '*status', and must free 'status->rmps'.
* '*status' is indeterminate if the return value is non-zero. */
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index de078b7..a88ef4c 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -264,6 +264,7 @@ void ofproto_port_set_cfm(struct ofproto *, ofp_port_t
ofp_port,
const struct cfm_settings *);
void ofproto_port_set_bfd(struct ofproto *, ofp_port_t ofp_port,
const struct smap *cfg);
+bool ofproto_port_bfd_status_changed(struct ofproto *, ofp_port_t ofp_port);
int ofproto_port_get_bfd_status(struct ofproto *, ofp_port_t ofp_port,
struct smap *);
int ofproto_port_is_lacp_current(struct ofproto *, ofp_port_t ofp_port);
@@ -422,6 +423,7 @@ struct ofproto_cfm_status {
size_t n_rmps;
};
+bool ofproto_port_cfm_status_changed(struct ofproto *, ofp_port_t ofp_port);
int ofproto_port_get_cfm_status(const struct ofproto *,
ofp_port_t ofp_port,
struct ofproto_cfm_status *);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 9764c1f..994e1c9 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1893,8 +1893,7 @@ iface_refresh_netdev_status(struct iface *iface)
static void
iface_refresh_ofproto_status(struct iface *iface)
{
- struct smap smap;
- int current, error;
+ int current;
if (iface_is_synthetic(iface)) {
return;
@@ -1909,15 +1908,21 @@ iface_refresh_ofproto_status(struct iface *iface)
ovsrec_interface_set_lacp_current(iface->cfg, NULL, 0);
}
- iface_refresh_cfm_stats(iface);
+ if (ofproto_port_cfm_status_changed(iface->port->bridge->ofproto,
+ iface->ofp_port)) {
+ iface_refresh_cfm_stats(iface);
+ }
- smap_init(&smap);
- error = ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
- iface->ofp_port, &smap);
- if (error >= 0) {
+ if (ofproto_port_bfd_status_changed(iface->port->bridge->ofproto,
+ iface->ofp_port)) {
+ struct smap smap;
+
+ smap_init(&smap);
+ ofproto_port_get_bfd_status(iface->port->bridge->ofproto,
+ iface->ofp_port, &smap);
ovsrec_interface_set_bfd_status(iface->cfg, &smap);
+ smap_destroy(&smap);
}
- smap_destroy(&smap);
}
/* Writes 'iface''s CFM statistics to the database. 'iface' must not be
@@ -1931,9 +1936,7 @@ iface_refresh_cfm_stats(struct iface *iface)
error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
iface->ofp_port, &status);
- if (error < 0) {
- /* Do nothing if there is no status change since last update. */
- } else if (error > 0) {
+ if (error > 0) {
ovsrec_interface_set_cfm_fault(cfg, NULL, 0);
ovsrec_interface_set_cfm_fault_status(cfg, NULL, 0);
ovsrec_interface_set_cfm_remote_opstate(cfg, NULL);
--
1.7.9.5
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
