On Tue, 29 Jan 2013 14:43:45 -0500 Dwight Engen <dwight.en...@oracle.com> wrote:
> Hi Natanael, thanks for the review! > > On Tue, 29 Jan 2013 09:46:29 +0100 > Natanael Copa <nc...@alpinelinux.org> wrote: > > [...] > > > --- /dev/null > > > +++ b/src/lua-lxc/Makefile.am > > > @@ -0,0 +1,26 @@ > > > +if ENABLE_LUA > > > + > > > +luadir=$(datadir)/lua/5.1 > > > +sodir=$(libdir)/lua/5.1/lxc > > > > maybe use something like: > > > > luadir=$(pkg-config --variable INSTALL_LMOD $(LUAPKGCONFIG)) > > sodir=$(pkg-config --variable INSTALL_CMOD $(LUAPKGCONFIG))/lxc > > > > It depends on the LUAPKGCONFIG variable above... > > I didn't really like this either but I didn't know what else to do: I > did look into the pkg-config variables available, and those variables > are not set in lua.pc on Fedora or Oracle Linux. That explains. The upstream lua.pc provides those. Fedora has its own makefiles (using autotools) and they appear to replace the upstream provided lua.pc. http://pkgs.fedoraproject.org/cgit/lua.git/tree/lua-5.1.4-autotoolize.patch#n31983 > Do you know of another lua binding that may give a good example of how > this should be determined? Not really. It does seam to provide the variable V which should return '5.1'. > [...] > > > +local core = require("lxc.core") > > > +local lfs = require("lfs") > > > +local table = require("table") > > > +local string = require("string") > > > +local io = require("io") > > > +module("lxc", package.seeall) > > > > I think module( ... , package.seeall) is not available in lua 5.2. I > > know you focus on 5.1 for now but... > > I originally didn't have the seeall, but I grew weary of putting io., > string., and table. in front of so many things as it cluttered the > code. I did not do anything with 5.2, so if we need to get rid of > seeall for that, it shouldn't be hard to do. We can do that later on yes. > > > +function string:split(delim, max_cols) > > > + local cols = {} > > > + local start = 1 > > > + local nextc > > > + repeat > > > + nextc = string.find(self, delim, start) > > > + if (nextc and #cols ~= max_cols - 1) then > > > + table.insert(cols, string.sub(self, start, nextc-1)) > > > + start = nextc + #delim > > > + else > > > + table.insert(cols, string.sub(self, start, > > > string.len(self))) > > > + nextc = nil > > > + end > > > + until nextc == nil or start > #self > > > + return cols > > > +end > > > + > > > +function dirname(path) > > > + local f,output > > > + f = io.popen("dirname " .. path) > > > + output = f:read('*all') > > > + f:close() > > > + return output:sub(1,-2) > > > +end > > > + > > > +function basename(path, suffix) > > > + local f,output > > > + f = io.popen("basename " .. path .. " " .. (suffix or "")) > > > + output = f:read('*all') > > > + f:close() > > > + return output:sub(1,-2) > > > +end > > > > We must be able to do better than popen for dirname/basename. > > > > What about using microlight? > > http://stevedonovan.github.com/microlight/ > > > > local ml = require('ml') > > I was trying to avoid use of modules that are not part of standard > distributions. As far as I can tell neither microlight nor penlight > are packaged. I did try a few string recipes that I found, but they did > not work with all inputs. dirname is only called once to find the path > to the cgroup mount, so I didn't worry about the performance. Makes sense. > I suppose we could just bring splitpath over (license allowing). Its a MIT license so it should be ok. https://github.com/stevedonovan/Microlight/blob/master/ml.lua#L119 Seems like you never use basename so we could make a string editing dirname only. It is only used for /proc/mounts so we can assume no relative paths (eg ./somedir), no trailing slashes (eg boot/) and no multiple slashes (eg /usr////bin). But we can expect a single '/'. I think this should work for our case. Note that you get the trailing '/' in there but it should not hurt us. function dirname(abspath) local i = #abspath local ch = abspath:sub(i,i) while i > 0 and ch ~= '/' do i = i - 1 ch = abspath:sub(i,i) end return abspath:sub(1,i) end In any case, I think the patch could be applied as is and I could post improvement patches later. Thanks! -nc ------------------------------------------------------------------------------ Everyone hates slow websites. So do we. Make your web apps faster with AppDynamics Download AppDynamics Lite for free today: http://p.sf.net/sfu/appdyn_d2d_jan _______________________________________________ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
