Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bindings/lua: convert to jansson, remove json-c #1522

Merged
merged 4 commits into from May 16, 2018

Conversation

Projects
None yet
4 participants
@grondo
Copy link
Contributor

grondo commented May 16, 2018

This PR converts Lua bindings to use jansson only and removes json-c.

The majority of the change was to replace json-lua.[ch] with jansson-lua.[ch] from flux-sched/rdl, and subsequent update of all users. The flux-lua.c bindings were largely converted away from using json objects directly, and instead mostly interfaces with jansson-lua via json strings.

Unfortunately the conversion ends up as one big commit since it cannot be done piecemeal.

Finally, to augment testing, a minor improvement was made to f:recvmsg() and flux.new() to allow the "loop" connector to be used in lua/t0001-send-recv.t to test the msg:respond() method.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 16, 2018

Really high value cleanup IMHO!

@grondo grondo force-pushed the grondo:lua-jansson branch from e1513e0 to 1267aec May 16, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 16, 2018

Coverage Status

Coverage decreased (-0.02%) to 78.86% when pulling bfb3644 on grondo:lua-jansson into fdf0891 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 16, 2018

Rebased on current master

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 16, 2018

Restarted one builder that hung right after the 'issues' test.

@garlick
Copy link
Member

garlick left a comment

Just a couple of places where a failure check is in order I think.

}

int lua_is_json_null (lua_State *L, int index)
{
return (lua_touserdata (L, index) == json_null);
}

int json_object_to_lua (lua_State *L, json_object *o)
int json_object_to_lua (lua_State *L, json_t *o)
{
if (o == NULL)
lua_pushnil (L);

This comment has been minimized.

@garlick

garlick May 16, 2018

Member

When o is NULL, should the function return after the pushnil?

This comment has been minimized.

@grondo

grondo May 16, 2018

Author Contributor

good catch! json_typeof() dereferences o so this would have caused a segfault. json-c json_object_get_type must have been resilient to NULL. Ugh. I wonder how many other bugs like this are lurking.

{
int rc;
json_object *o = json_object_new_array ();
json_t *o = json_array ();

This comment has been minimized.

@garlick

garlick May 16, 2018

Member

missing a check for o == NULL


if (lua_isnoneornil (L, i))
return (-1);

switch (lua_type (L, index)) {
case LUA_TNUMBER:
if (lua_is_integer (L, index))
o = json_object_new_int64 (lua_tointeger (L, index));
o = json_integer (lua_tointeger (L, index));

This comment has been minimized.

@garlick

garlick May 16, 2018

Member

Each of these json_object functions can return NULL. Should that be checked for and luaL_error() be used to set an error? (Not sure - since result is NULL, maybe that is sufficient?)

This comment has been minimized.

@grondo

grondo May 16, 2018

Author Contributor

The NULL should be sufficient, but the function would return success (0) even if o was NULL. Will fix.

This comment has been minimized.

@grondo

grondo May 16, 2018

Author Contributor

BTW, meant to mention that luaL_error() could also work here, but that throws an error in the lua interpreter and immediately stops the script. The current method is to return nil, error_message to the caller, which would potentially allow the script to recover or issue a different error message, etc. I'm actually kind of ambivalent which way to go, but since the existing behavior was to return error to the caller, we can keep it that way for now and rethink the approach when the bindings are updated.

@@ -243,17 +235,17 @@ static json_object * lua_table_to_json (lua_State *L, int index)
if (lua_table_is_array (L, index))
return lua_table_to_json_array (L, index);

o = json_object_new_object ();
o = json_object ();

This comment has been minimized.

@garlick

garlick May 16, 2018

Member

Needs a check for o == NULL

|| (json_str && !(zi->o = json_tokener_parse (json_str)))) {
free (zi->tag);
free (zi);
zi->msg = flux_msg_copy (*msg, true);

This comment has been minimized.

@garlick

garlick May 16, 2018

Member

needs a check for null

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 16, 2018

Thanks for the review! I'll get all those fixed up asap

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 16, 2018

Ok, check out my quick work pushed as a separate commit here. If it looks ok I'll squash. Thanks!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #1522 into master will decrease coverage by <.01%.
The diff coverage is 88.79%.

@@            Coverage Diff             @@
##           master    #1522      +/-   ##
==========================================
- Coverage   78.58%   78.57%   -0.01%     
==========================================
  Files         165      165              
  Lines       30823    30801      -22     
==========================================
- Hits        24222    24202      -20     
+ Misses       6601     6599       -2
Impacted Files Coverage Δ
src/bindings/lua/kvs-lua.c 76.59% <ø> (ø) ⬆️
src/bindings/lua/zmsg-lua.c 84.26% <85.18%> (+1.71%) ⬆️
src/bindings/lua/jansson-lua.c 83.47% <86.95%> (ø)
src/bindings/lua/flux-lua.c 82.19% <93.02%> (+0.52%) ⬆️
src/broker/module.c 83.79% <0%> (-1.68%) ⬇️
src/broker/content-cache.c 73.43% <0%> (-1.3%) ⬇️
src/common/libflux/mrpc.c 85.49% <0%> (-1.18%) ⬇️
src/common/libflux/message.c 81.29% <0%> (+0.11%) ⬆️
... and 3 more

@grondo grondo requested a review from garlick May 16, 2018

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 16, 2018

Looks good.

grondo added some commits May 15, 2018

bindings/lua: drop json-c, use jansson
Replace json-lua.[ch] with jansson-lua.[ch] and update all users.
bindings/lua: allow flux.new() to take string argument
Problem: the flux.new() Lua constructor only allows passing NULL
argument to flux_open(3), which doesn't allow Lua bindings to be
used with alternate connectors or paths.

Add a simple optional string argument to l_flux_new() and pass
directly to flux_open(3).
bindings/lua: allow f:recvmsg() to receive any message
The `match` argument in f:recvmsg() restricts the function to only
receiving response messages. It would be nice to be able to receive
requests with this function for testing, so relax the default match
structure to FLUX_MATCH_ANY.
testsuite: lua/t0001-send-recv.t: test msg:respond()
Extend the t0001-send-recv.t Lua test to test msg:respond() by
opening a flux handle using the "loop" connector, and sending a
request/response across the connector using msg:respond().

@grondo grondo force-pushed the grondo:lua-jansson branch from b3b8bf1 to bfb3644 May 16, 2018

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 16, 2018

Thanks! Squashed the fixups into the main commit and force-pushed.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented May 16, 2018

Really nice to get this done!

@garlick garlick merged commit 03002f3 into flux-framework:master May 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 78.86%
Details
@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented May 16, 2018

Thanks!

@grondo grondo deleted the grondo:lua-jansson branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.