On Sun, Jun 01, 2014 at 10:04:28PM -0700, Alex Wang wrote:
> 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);
Since those are all a sequence of mutex_lock(), get some value,
mutex_unlock, it might be a good optimization to have a function
that get all the status at once, e.g.:
cfm_get_status(cfm, &status)
{
mutex_lock()
get all status
mutex_unlock()
}
> } 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. */
nitpick: add 'either' or a second 'if' before 'CFM'.
Thanks,
fbl
> + 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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
