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

[Natanael Copa]

On Tue, 29 Jan 2013 14:43:45 -0500
Dwight Engen <dwight.engen at oracle.com> wrote:

> 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.

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