On Thu, Aug 1, 2013 at 11:41 AM, Lasse Karstensen <span dir="ltr"><<a href="mailto:lkarsten@varnish-software.com" target="_blank">lkarsten@varnish-software.com</a>></span> wrote:<br><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On Thu, Aug 01, 2013 at 02:03:14AM +0100, Federico Schwindt wrote:<br>
> On Wed, Jul 31, 2013 at 2:46 PM, Lasse Karstensen <<br>> <a href="mailto:lkarsten@varnish-software.com" target="_blank">lkarsten@varnish-software.com</a>> wrote:<br></div><div>> > I've extended the std vmod to include an ip() method, which<br>
> > converts a string into VCL IP. The result can be used for<br>> > matching against a VCL ACL.<br></div><div>> A few comments:<br>> - rp leaks if WS_Reserve() fails.<br>> - WS_Reserve() is cheaper that getaddrinfo(), so I'd check first if there<br>
> is space and then do the getaddrinfo() call. That'd simplify the error path<br>> too.<br>> - Missing CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC).<br>> - You could just check for getaddrinfo() != 0 instead of using s = .. since<br>
> it's not used anywhere else.<br><br></div>Hi Federico.<br>Thank you for taking the time to review this.<br><br>I've implemented the changes you proposed. Updated (full) patch is attached.</blockquote><div><br>
</div></div><div>Style aside looks good to me.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>> - Using VCL_IP for the fallback parameter restricts what you can use to<br>> client.ip or server.ip. This might or might not be a problem.<br>> I wrote a similar function a while ago that was using a STRING parameter as<br>
> suggested by Tollef. Not sure if this is still required.<br><br></div>Yes, we discussed this for a bit in the office.<br>I don't have strong opinions on either side.<br><br>You can of course nest them to get an arbitrary fallback:<br>
        std.ip(req.http.X-Forwarded-For, std.ip("127.255.255.255"));</blockquote><div><br></div></div><div>I take you meant:</div><div><br></div><div>std.ip(req.http.X-Forwarded-For, std.ip("127.255.255.255", server.ip));</div>
<div><br></div><div>or similar above.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The error handling when the fallback conversion fails doesn't seem to have<br>any obvious solution. If the user gets to pick a fallback by him/herself,<br>then at least they know clearly what to check for.</blockquote>
<div><br></div></div><div>True. Perhaps this is a good candidate for the VCL Examples wiki page.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>f.-</div></font></span></div>