From 8f5514fe7c3c2ba0d06405c1f75e08ae78e65ee9 Mon Sep 17 00:00:00 2001 From: Alex Wang Date: Fri, 30 May 2014 15:07:31 -0700 Subject: [PATCH] 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 Acked-by: Ben Pfaff --- ofproto/ofproto-dpif.c | 33 ++++++++++++++++++++------------- ofproto/ofproto-provider.h | 25 +++++++++++++------------ ofproto/ofproto.c | 32 +++++++++++++++++++++++++++----- ofproto/ofproto.h | 3 +++ vswitchd/bridge.c | 25 ++++++++++++++----------- 5 files changed, 77 insertions(+), 41 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 9aa125545..9e4a45588 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 { @@ -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 6e8cd9b8f..2ba4f9f59 100644 --- 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 fb9313b90..e5de16e45 100644 --- 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 5f5e6c8ed..60b742bcc 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); @@ -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 014ef6f8b..eee48694e 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); -- 2.20.1