Re: Please comment on these Lua to C library bindings that I wrote

[Jerome Vuarand <jerome.vuarand@...>]

2012/11/20 Richard W.M. Jones <rjones@redhat.com>:
> On Tue, Nov 20, 2012 at 02:23:39PM +0000, Jerome Vuarand wrote:
>> 2012/11/20 Richard W.M. Jones <rjones@redhat.com>:
>> >> And as for the "single change", it has a big impact. Whether you pass
>> >> a valid string or NULL, luaL_register will create a new table on the
>> >> stack or not.
>> >
>> > I think what I don't understand is do I need to add this string
>> > parameter?  The code seems to work fine at the moment (using NULL), so
>> > I'm inclined to just leave it alone.
>>
>> You don't "need" to, but the luaL_register call does two very
>> different things whether you pass a string or not. Both are
>> acceptable, depending on your intent. But I don't know your intent
>> there. A simple luaopen_ function creates a Lua table, put some stuff
>> in that table, and somehow pass it back to require. That table is then
>> called a "module". Your luaopen_ function does something completely
>> different. It is perfectly valid, provided you understand what you are
>> doing.
>>
>> To sum it up, what do you intend to do with that call to luaL_register?
>
> I think to a first approximation I have no idea what I'm doing.
>
> What I'm *trying* to do is to let people call:
>
>   g = Guestfs.create ()
>
> in order to create a libguestfs handle which then has methods like
> g:set_verbose(true) etc., plus a few "global" variables like
> Guestfs.event_all.
>
> All the above works fine with the code I've written.
>
> But from what you say it sounds like what I'm doing is completely wrong.
>
> Is there an example of a luaopen_* function which does this right?
> Or can you tell me what specifically I'm doing wrong?

Well, there is nothing wrong there. It works after all. But your code
has subtle side effects that you may want to avoid. Here is a small
list of things that distub me (it's a purely personal opinion):

- you require your module with the name "guest", and the global
variable used to access it is named "Guest" (Lua is case sensitive)
- after you call g1 = Guest.create(), you can call g2 = g1.create()
- you expose Guest.__gc (and Guest.__index), and especially you let
people remove them (with whatever consequences there is if your
finalizers are not called)

This is all because you have one table for three purposes: a module, a
metatable, and a method table.

If I were you I would create three tables, one for each purpose. Also
I would avoid hardcoding the name of the library (be it "guest" or
"Guest"), require pass you the module name. For example:

luaL_Reg metamethods[] = {
  {"__gc", ...},
  {0, 0},
};

luaL_Reg methods[] = {
  {"set_verbose", ...}
  {0, 0},
};

luaL_Reg functions[] = {
  {"create", ...},
  {0, 0},
};

int
luaopen_guestfs (lua_State *L)
{
  char v[256];

  /* Create metatable. */
  luaL_newmetatable (L, LUA_GUESTFS_HANDLE); /* mt */
  luaL_register (L, NULL, metamethods);      /* mt */

  /* Create methods table. */
  lua_newtable (L);                          /* mt, methods */
  luaL_register (L, NULL, methods);          /* mt, methods */

  /* Set __index field of metatable to point to the methods table. */
  lua_setfield (L, -1, "__index");           /* mt */

  /* Pop metatable, it is no longer needed */
  lua_pop(L, 1);                             /* <empty> */

  /* Create module table */
  luaL_register (L, lua_tostring(L, 1), functions); /* module */

  /* Globals in the Guestfs.* namespace. */
  lua_pushliteral (L, "event_all");          /* module, k */
  push_string_list (L, (char **) event_all); /* module, k, v */
  lua_settable (L, -3);                      /* module */

  /* Add _COPYRIGHT, etc. fields to the metatable. */
  lua_pushliteral (L, "_COPYRIGHT");         /* module, k */
  lua_pushliteral (L, "Copyright (C) 2009-2012 Red Hat Inc."); /*
module, k, v */
  lua_settable (L, -3);                      /* module */

  lua_pushliteral (L, "_DESCRIPTION");       /* module, k */
  lua_pushliteral (L, "Lua binding to libguestfs"); /* module, k, v */
  lua_settable (L, -3);                      /* module */

  lua_pushliteral (L, "_VERSION");           /* module, k */
  make_version_string (v, sizeof v);
  lua_pushlstring (L, v, strlen (v));        /* module, k, v */
  lua_settable (L, -3);                      /* module */

  /* Don't return anything, the luaL_register created a global */
  return 0;
}

Then in your user code you can write:

require "guest"

g1 = guest.create() -- note the lowercase "guest", to match the module name

Also, as an extra step, to be a bit future-proof, I would replace
luaL_register (L, lua_tostring(L, 1), functions); with:

lua_newtable (L);
luaL_register (L, NULL, functions);

And and the end of luaopen return 1 instead of 0. This would avoid the
creation of the global variable. The module would then have to be used
like that:

local Guest = require "guest" -- no the use of the return value

g1 = Guest.create() -- Guest is not a global, but a local variable
with whatever name you used above