Hello!
On Tue, Dec 17, 2013 at 6:33 AM, Aapo Talvensaari wrote:
> I started to build a few modules for OpenResty. Currently I have done these:
>
> https://github.com/bungle/lua-resty-random (LuaJIT based random library)
> https://github.com/bungle/lua-resty-scrypt (LuaJIT based scrypt library)
> https://github.com/bungle/lua-scrypt (C-module for lua-resty-scrypt)
>
Great! Thank you for your contributions!
> I attached a patch file to OpenResty configure-file for these (in this
> mail). These modules are the first ever that I have done for OpenResty, and
> they are considered version 0.0.1 quality.
>
Libraries bundled in OpenResty requires careful code review and
testing on my side, which means long delay :P
Actually I'm thinking about setting out a centralized package
management web site and related toolchain for 3rd-party libraries for
ngx_lua/openresty. Kinda like CPAN and LuaRocks. So that everyone can
publish their libraries and everyone can install the libraries with a
single command, like
$ lresty resty.scrypt
> Current the lua-resty-random library has a naming conflict with
> lua-resty-string library (both have random.lua ->
> --without-lua-resty-string) , so maybe they could be merged, or I will
> rename mine.
>
Yeah, module name conflicting is bad. Maybe we can collaborate here?
> I think this was interesting task to do, and I'm looking forward to make
> more libraries. Currently I'm thinking about implementing lua-resty-session
> (both stateless, and statefull), lua-resty-cookie (cookie helper lib),
> lua-resty-auth (usename/password, and maybe oauth, openid, facebook login
> etc. support), and some others (libxl, and maybe some pdf-lib too).
Awesome! Keep them coming!
> But as
> this is rather new to me, I would like to get some feedback about those that
> I have already done.
>
I hope you can create a test suite for your libraries based on my
Test::Nginx test scaffold. You can take a look at my existing
lua-resty-* libraries for examples, like lua-resty-redis.
Also, please use "local" to declare your local Lua variables in
functions wherever possible. For example, in your "token" function in
your resty.random module, the variables "n", "count", "token" are all
global by accident. BTW, you can use the lua-releng tool to analyze
your Lua source for such issues. See
https://github.com/chaoslawful/lua-nginx-module#lua-variable-scope
and
https://github.com/agentzh/nginx-devel-utils/blob/master/lua-releng
> One more question, though. Right now I have this line in scrypt.lua:
> local scrypt = ffi.load("/usr/local/openresty/lualib/scrypt.so")
>
> This was the only way I figured out how to load that scrypt.so on my Mac
> (inside OpenResty).
I think you can just search the paths in package.cpath and build the
absolute path to the scrypt.so file yourself in Lua. And then feed the
path to ffi.load().
BTW, /usr/local/openresty/lualib/?.so should already in package.cpath
if you're using the OpenResty bundle, and you don't have to specify
that explicitly via the lua_package_cpath directive. You can dump out
the existing cpath by print out the package.cpath thing in Lua.
There're some other places in your resty.random module that deserve
optimizing or tweaking:
1. "local p = ffi_new("int[?]", 1)" in a Lua function is expensive. I
think you can create such buffers on the toplevel of your module and
reuse it everytime you need it.
2. Don't call ffi_load in your function bytes everytime it is gets
called.You should load it only once.
3. instead of opening and closing the file /dev/urandom, maybe you can
just cache the file handle on the toplevel of your Lua module?
4. I think you'd better local'ize things like math.randomseed() and
table.concat() on the toplevel of your Lua module to save the table
gets in your Lua function calls.
5. in your function token, maybe you should put the alphabet table
{'A', 'B', 'C', 'D', ... } onto your module's toplevel to prevent
duplicating it everytime your "token" function gets called?
6. For the coding style, I hope you can eliminate source code lines
longer than 80 columns ;)
Best regards,
-agentzh