[lxc-devel] [PATCH v2] add lua binding for the lxc API

[Dwight Engen]

Hi Natanael, thanks for the review!

On Tue, 29 Jan 2013 09:46:29 +0100
Natanael Copa <ncopa at 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