Hello Donald, I understood that the way this function was used, was like the other vty conversion functions. - get afi/safi values - get afi/safi/vrf values. meaning that the caller of the function must ensure that the values he passes as params have good initial values. I would have done a little change. This is because you handle pointers in that function (*vrf), and you assume that this pointer is valid. Whereas an user that does not want to care about VRF, does not want to initialise a specific value. he uses NULL as parameter. For that, I would have ignored vrf case ( returned 0 if a vrf is found, and return ok if no vrf is found). The rest of the series is ok for me. Regards, Philippe On Thu, Jan 26, 2017 at 3:21 PM, Donald Sharp <sharpd@cumulusnetworks.com> wrote:
https://github.com/freerangerouting/frr/pull/107
Philippe -
You had a review comment two days ago:
how do you initialise vrf in the case vrf_name is already null ( commands with no vrf at all)? I would lie the *vrf value to be set inside that function.
I responded with:
I do not believe this is the right thing to do. Only the calling function knows what the right defaults are for the afi/safi/vrf due to we assume different defaults based upon the command. This approach would break the ability for the calling function to do the right thing. As I stated I will update the Pull Request with better documentation for this funciton.
I've also added a commit that updates the function to further explain it. Is there anything else you would like to have done?
donald