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