| Message ID | 1445537282-1197-1-git-send-email-azhou@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
> On Oct 22, 2015, at 11:08 AM, Andy Zhou <azhou@nicira.com> wrote: > > @@ -321,11 +321,8 @@ bfd_get_status(const struct bfd *bfd, struct smap *smap) > smap_add(smap, "state", bfd_state_str(bfd->state)); > smap_add(smap, "diagnostic", bfd_diag_str(bfd->diag)); > smap_add_format(smap, "flap_count", "%"PRIu64, bfd->flap_count); > ... > + smap_add(smap, "remote_state", bfd_state_str(bfd->rmt_state)); > + smap_add(smap, "remote_diagnostic", bfd_diag_str(bfd->rmt_diag)); > ovs_mutex_unlock(&mutex); The man page describing "diagnostic" and "remote_diagnostic" make it sound like they're only populated on error. It may be worth taking the language from the RFC. So, "diagnostic" would look along the lines of: "A diagnostic code specifying the local system's reason for the last change in local BFD session state. The error messages are defined in section 4.1 of [RFC 5880]." And "remote_diagnostic" would look along the lines of: "A diagnostic code specifying the local system's reason for the last change in remote BFD session state. The error messages are defined in section 4.1 of [RFC 5880]." Acked-by: Justin Pettit <jpettit@nicira.com> --Justin
On Thu, Oct 22, 2015 at 5:56 PM, Justin Pettit <jpettit@nicira.com> wrote: > >> On Oct 22, 2015, at 11:08 AM, Andy Zhou <azhou@nicira.com> wrote: >> >> @@ -321,11 +321,8 @@ bfd_get_status(const struct bfd *bfd, struct smap *smap) >> smap_add(smap, "state", bfd_state_str(bfd->state)); >> smap_add(smap, "diagnostic", bfd_diag_str(bfd->diag)); >> smap_add_format(smap, "flap_count", "%"PRIu64, bfd->flap_count); >> ... >> + smap_add(smap, "remote_state", bfd_state_str(bfd->rmt_state)); >> + smap_add(smap, "remote_diagnostic", bfd_diag_str(bfd->rmt_diag)); >> ovs_mutex_unlock(&mutex); > > The man page describing "diagnostic" and "remote_diagnostic" make it sound like they're only populated on error. It may be worth taking the language from the RFC. So, "diagnostic" would look along the lines of: > > "A diagnostic code specifying the local system's reason for the last change in local BFD session state. The error messages are defined in section 4.1 of [RFC 5880]." > > And "remote_diagnostic" would look along the lines of: > > "A diagnostic code specifying the local system's reason for the last change in remote BFD session state. The error messages are defined in section 4.1 of [RFC 5880]." Make sense. I will send out a separate patch to update the man page. Thanks. > > Acked-by: Justin Pettit <jpettit@nicira.com> > > --Justin > > Thanks for the review. Pushed to master.
diff --git a/lib/bfd.c b/lib/bfd.c index dae682f..adac666 100644 --- a/lib/bfd.c +++ b/lib/bfd.c @@ -1,4 +1,4 @@ -/* Copyright (c) 2013, 2014 Nicira, Inc. +/* Copyright (c) 2013, 2014, 2015 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -321,11 +321,8 @@ bfd_get_status(const struct bfd *bfd, struct smap *smap) smap_add(smap, "state", bfd_state_str(bfd->state)); smap_add(smap, "diagnostic", bfd_diag_str(bfd->diag)); smap_add_format(smap, "flap_count", "%"PRIu64, bfd->flap_count); - - if (bfd->state != STATE_DOWN) { - smap_add(smap, "remote_state", bfd_state_str(bfd->rmt_state)); - smap_add(smap, "remote_diagnostic", bfd_diag_str(bfd->rmt_diag)); - } + smap_add(smap, "remote_state", bfd_state_str(bfd->rmt_state)); + smap_add(smap, "remote_diagnostic", bfd_diag_str(bfd->rmt_diag)); ovs_mutex_unlock(&mutex); }
RFC 5880 specified bfd.RemoteSessionState as one of the state variables. In OVS implementation, this value is exported to OVSDB's BFD status column of the interface table, as one of the map elements, with the key of 'remote_state'. It can be surprising when the 'remote_state' map element disappears when BFD is in the 'DOWN' state, but otherwise always exported. Change to always exporting it, to make it more predictable for applications that monitors the BFD status column. While at it, make the same change to 'remote_diagnostic', so that it is also always exported to OVSDB for consistency. VMWare-BZ: 1535979 Reported-by: Mihir Gangar <gangarm@vmware.com> Signed-off-by: Andy Zhou <azhou@nicira.com> --- lib/bfd.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)