Thanks for the review, the sense of getting closer is coming to me, ;D
Please see my reply inline,
+ /* 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.
>>
>
> Perhaps you could clarify that '*status' is indeterminate if this function
> returns non-zero, just to be crystal clear?
>
Thx, I'll add that.
> Also, minor formatting thing: "EOPNOTSUPP..." could go on a new line
> like other functions in the file.
> (this comment goes for BFD in ofproto-provider.h too)
>
>
Yeah, I'll do that,
>
>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 436a745..86b4a02 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -1018,7 +1018,7 @@ ofproto_port_set_bfd(struct ofproto *ofproto,
>> ofp_port_t ofp_port,
>>
>> /* Populates 'status' with key value pairs indicating the status of the
>> BFD
>> * session on 'ofp_port'. This information is intended to be populated
>> in the
>> - * OVS database. Has no effect if 'ofp_port' is not na OpenFlow port in
>> + * OVS database. Has no effect if 'ofp_port' is not an OpenFlow port in
>> * 'ofproto'. */
>> int
>> ofproto_port_get_bfd_status(struct ofproto *ofproto, ofp_port_t ofp_port,
>>
>
> This version of the comment seems to be missing a bunch of information
> compared with the ofproto-provider.h one. Could you update it, too?
>
>
I'll update it,
>
>> struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
>> - return (ofport
>> - && ofproto->ofproto_class->get_cfm_status
>> - && ofproto->ofproto_class->get_cfm_status(ofport, status));
>> + return (ofport && ofproto->ofproto_class->get_cfm_status ?
>> + ofproto->ofproto_class->get_cfm_status(ofport, status)
>> + : EOPNOTSUPP);
>> }
>>
>
> I think the '?' usually goes at the start of the next line.
>
Yes, I'll fix it
>
>
>
>> @@ -1836,9 +1836,14 @@ iface_refresh_cfm_stats(struct iface *iface)
>> {
>> const struct ovsrec_interface *cfg = iface->cfg;
>> struct ofproto_cfm_status status;
>> + int error;
>>
>> - if (!ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
>> - iface->ofp_port, &status)) {
>> + error = ofproto_port_get_cfm_status(iface->port->bridge->ofproto,
>> + iface->ofp_port, &status);
>> + /* Do nothing if there is no status change since last update. */
>>
>
> This comment could be placed inside the first if condition below to
> replace that comment:
>
>
Sure, will do that,
> smap_init(&smap);
>> - ofproto_port_get_bfd_status(br->ofproto, iface->ofp_port,
>> - &smap);
>> + error = ofproto_port_get_bfd_status(br->ofproto,
>> iface->ofp_port,
>> + &smap);
>> + if (error < 0) {
>> + continue;
>>
>
> Should smap_destroy() also be called in this case?
>
There is no need in the current ovs. But I'll add it to prevent any
changes in the future,
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
