ofproto: Add separate functions for checking bfd/cfm status change.

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 <alexw@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 9aa1255..9e4a455 100644 (file)
--- 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 {
@@ -1809,6 +1806,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 cfm_status *status)
@@ -1817,11 +1822,7 @@ get_cfm_status(const struct ofport *ofport_,
     int ret = 0;
 
     if (ofport->cfm) {
-        if (cfm_check_status_change(ofport->cfm)) {
-            cfm_get_status(ofport->cfm, status);
-        } else {
-            ret = NO_STATUS_CHANGE;
-        }
+        cfm_get_status(ofport->cfm, status);
     } else {
         ret = ENOENT;
     }
@@ -1847,6 +1848,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)
 {
@@ -1854,11 +1863,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;
     }
@@ -4949,8 +4954,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 6e8cd9b..2ba4f9f 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1348,13 +1348,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 if 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
@@ -1372,12 +1372,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 if 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 fb9313b..e5de16e 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1001,10 +1001,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
@@ -3682,11 +3693,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 5f5e6c8..60b742b 100644 (file)
--- 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);
@@ -399,6 +400,8 @@ void ofproto_get_netflow_ids(const struct ofproto *,
 void ofproto_get_ofproto_controller_info(const struct ofproto *, struct shash *);
 void ofproto_free_ofproto_controller_info(struct shash *);
 
+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 cfm_status *);

diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 014ef6f..eee4869 100644 (file)
--- 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);