<div dir="ltr"><div><div><div><div><div>Hello Donald,<br><br></div>I understood that the way this function was used, was like the other vty conversion functions.<br></div>- get afi/safi values<br>- get afi/safi/vrf values.<br></div><div>meaning that the caller of the function must ensure that the values  he passes as params have good initial values.<br><br></div><div>I would have done a little change.<br></div><div>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.<br>For that, I would have ignored vrf case ( returned 0 if a vrf is found, and return ok if no vrf is found).<br><br></div><div>The rest of the series is ok for me.</div><br></div>Regards,<br><br></div>Philippe<br><div><div><div><br></div><div><br></div></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 26, 2017 at 3:21 PM, Donald Sharp <span dir="ltr"><<a href="mailto:sharpd@cumulusnetworks.com" target="_blank">sharpd@cumulusnetworks.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><a href="https://github.com/freerangerouting/frr/pull/107" rel="noreferrer" target="_blank">https://github.com/<wbr>freerangerouting/frr/pull/107</a><br>
<br>
Philippe -<br>
<br>
You had a review comment two days ago:<br>
<br>
how do you initialise vrf in the case vrf_name is already null (<br>
commands with no vrf at all)?<br>
I would lie the *vrf value to be set inside that function.<br>
<br>
I responded with:<br>
<br>
I do not believe this is the right thing to do. Only the calling<br>
function knows what the right defaults are for the afi/safi/vrf due to<br>
we assume different defaults based upon the command. This approach<br>
would break the ability for the calling function to do the right<br>
thing. As I stated I will update the Pull Request with better<br>
documentation for this funciton.<br>
<br>
I've also added a commit that updates the function to further explain<br>
it.  Is there anything else you would like to have done?<br>
<span class="HOEnZb"><font color="#888888"><br>
donald<br>
</font></span></blockquote></div><br></div>