Thanks for the review, Flavio~
> 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()
> }
>
>
Yes, I'll do it in a separate patch. You will see it in V2.
>
> > } 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'
>
Fixed, Thanks,
> Thanks,
> fbl
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
