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. Do you know of another
lua binding that may give a good example of how this should be
determined?
[...]
> > +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.
> > +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. I suppose
we could just bring splitpath over (license allowing).
> function dirname(path)
> local dir, file = ml.splitpath(path)
> return dir
> end
>
> function basename(path)
> local dir, file = ml.splitpath(path)
> return file
> end
>
> Microlight also ships a split() implementation.
I've seen a few different lua split() implementations, all with subtle
differences :)
> -nc
------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
