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