Hello Dirk!
First of all, I must say sorry for the long delay on my side!
On Mon, Sep 10, 2012 at 7:14 AM, Dirk Feytons wrote:
>
> So here's a first attempt that seems to work for me.
>
> A couple of notes and questions:
> - I followed the LuaSocket API where you pass a string naming the
> option you want to set and then the value. Right now only "passcred"
> with a boolean value is accepted. Alternative could be to register the
> various supported options with their numeric value as
> ngx.socket.option.SO_PASSCRED etcetera. Any preference?
> - I directly included the various Linux header files defining
> setsockopt(), SO_PASSCRED and such. I suspect this should happen in a
> more portable way?
> - As mentioned in the commit comment setoption() can only be called
> after calling setpeername() because only then there's a valid
> ngx_http_lua_socket_udp_upstream_t*. Is this acceptable? Should we
> cache the option(s) and apply them when the socket is actually
> created? But then we can only do delayed error reporting...
>
I've just committed a patch based on your version:
https://github.com/chaoslawful/lua-nginx-module/commit/dfc7a4c
Basically I do not introduce a new method for this, but rather make it
happen automatically within the setpeername method. Also, this
autobind happens only on systems with SO_PASSCRED defined (I do the
feature test in "config"). So it still does not work on systems like
FreeBSD (yet).
In the future, I'd introduce a second argument to the setpeername()
method to allow the user specify the socket file path for the client
endpoint, so that datagram unix domain sockets can work on other
systems lacking support for autobind.
Do you like it?
> And some more generic questions regarding the cosocket code:
> - Do you feel it's really necessary to strictly check the number of
> arguments with which a Lua C function is called? I'm more inclined to
> just ignore extraneous arguments. Less code for me to write and
> maintain, and a smaller binary. That's also how lots of other Lua
> libraries are written, including Lua's standard libraries.
I'm adding this check to help beginners diagnose problems and also
reduce the risk that existing user Lua code breaks when I add new
arguments to the API in a new version of ngx_lua.
I think it is fine for the standard Lua implementation to omit such
checks because it does have a fixed ABI.
> - I saw some calls to lua_tonumber() that should be lua_tointeger() I
> think. I can change it and send a pull request.
Yes please :)
> - In ngx_http_lua_socket_udp_setpeername() there's a call to
> ngx_http_lua_check_context(); is this really needed?
We should probably move such checks from setpeername() to receive().
In theory, ngx.socket.udp()'s setpeername() and send() could be used
in all contexts including log_by_lua*, which could be very useful.
> - I see some opportunity to reduce the amount of string data from
> error traces by parameterizing them. E.g. write luaL_error(L, "no %s
> found", "ctx")/luaL_error(L, "no %s found", "request") instead of "no
> ctx found" and "no request found". One disadvantage of this is that
> you can't grep the code for an error string anymore. Would you be
> willing to accept such changes?
>
Not really :)
> (I'm used to develop for embedded systems; can you tell? :))
>
Yes, I can :)
I apologize again for replying so late!
Best regards,
-agentzh
