crypto_acompress #311
crypto_acompress #311
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the synchronous compression implementation with an asynchronous one using the Linux kernel's acompress API. The change introduces a new low-level crypto.acompress module that provides asynchronous compression operations, while maintaining backward compatibility through a high-level crypto.comp Lua module that wraps the async operations to provide a synchronous interface.
Key changes:
- Replaces synchronous
luacrypto_comp.cwith asynchronousluacrypto_acompress.c - Adds high-level Lua wrapper
crypto/comp.luato maintain synchronous API compatibility - Updates build configuration to use the new acompress module
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/luacrypto_comp.c | Removed - old synchronous compression implementation |
| lib/luacrypto_acompress.c | Added - new asynchronous compression implementation using kernel acompress API |
| lib/crypto/comp.lua | Added - high-level Lua wrapper providing synchronous interface over async operations |
| tests/crypto/comp.lua | Minor error message improvement for clarity |
| config.ld | Updated documentation config to reference new files |
| Makefile | Updated build config to use acompress module |
| Kbuild | Updated kernel build config to use acompress module |
jperon
force-pushed
the
jperon_crypto_acompress
branch
from
723e663 to
1df9379
Compare
jperon
force-pushed
the
jperon_crypto_acompress
branch
from
9a9b7c6 to
c0e8987
Compare
jperon
force-pushed
the
jperon_crypto_acompress
branch
from
c0e8987 to
59c56ec
Compare
| static const lunatik_class_t luacrypto_acompress_classes[] = { | ||
| luacrypto_acompress_class, | ||
| luacrypto_acompress_req_class, | ||
| {NULL} |
There was a problem hiding this comment.
| {NULL} | |
| NULL |
shouldn't be just NULL, here? the array element is a pointer, not a struct, right?
| /* Table to hold all references */ | ||
| lua_newtable(L); | ||
|
|
||
| lua_pushvalue(L, 4); | ||
| lua_setfield(L, -2, "cb"); | ||
|
|
||
| lua_pushvalue(L, 2); | ||
| lua_setfield(L, -2, "buf"); | ||
|
|
||
| lua_pushvalue(L, 1); | ||
| lua_setfield(L, -2, "self"); | ||
|
|
||
| obj->refs = luaL_ref(L, LUA_REGISTRYINDEX); |
There was a problem hiding this comment.
I think moving this to an auxiliary function, also renaming ref to request (or another name for this table), would make it more readable.. btw, can't we use the object pointer to register this table instead of using a new ref on Lua registry? I would also move the call to this aux function to the macro bellow as it accesses argument indexes directly.. we are trying to reference the argument numbers on the lua_CFunction itself or passing the index to auxiliary functions to make easier to understand the context of the stack
| #define LUNATIK_NEWLIB(libname, funcs, class, namespaces) \ | ||
| int luaopen_##libname(lua_State *L); \ | ||
| int luaopen_##libname(lua_State *L) \ | ||
| { \ | ||
| const lunatik_class_t *cls = class; /* avoid -Waddress */ \ | ||
| const lunatik_namespace_t *nss = namespaces; /* avoid -Waddress */ \ | ||
| luaL_newlib(L, funcs); \ | ||
| if (cls) { \ | ||
| lunatik_checkclass(L, cls); \ | ||
| lunatik_newclass(L, cls); \ | ||
| } \ | ||
| if (nss) \ | ||
| lunatik_newnamespaces(L, nss); \ | ||
| return 1; \ | ||
| } \ | ||
| static inline void lunatik_register_class(lua_State *L, const lunatik_class_t *class) | ||
| { | ||
| if (class) { | ||
| lunatik_checkclass(L, class); | ||
| lunatik_newclass(L, class); | ||
| } | ||
| } | ||
|
|
||
| static inline void lunatik_register_classes(lua_State *L, const lunatik_class_t *classes) | ||
| { | ||
| if (classes) | ||
| lunatik_newclasses(L, classes); | ||
| } | ||
|
|
||
| static inline void lunatik_register_namespaces(lua_State *L, const lunatik_namespace_t *namespaces) | ||
| { | ||
| if (namespaces) | ||
| lunatik_newnamespaces(L, namespaces); | ||
| } | ||
|
|
||
| #define LUNATIK_NEWLIB(libname, funcs, class, namespaces) \ | ||
| int luaopen_##libname(lua_State *L); \ | ||
| int luaopen_##libname(lua_State *L) \ | ||
| { \ | ||
| luaL_newlib(L, funcs); \ | ||
| lunatik_register_class(L, class); \ | ||
| lunatik_register_namespaces(L, namespaces); \ | ||
| return 1; \ | ||
| } \ | ||
| EXPORT_SYMBOL_GPL(luaopen_##libname) | ||
|
|
||
| #define LUNATIK_NEWLIB_MULTICLASS(libname, funcs, classes, namespaces) \ | ||
| int luaopen_##libname(lua_State *L); \ | ||
| int luaopen_##libname(lua_State *L) \ | ||
| { \ | ||
| luaL_newlib(L, funcs); \ | ||
| lunatik_register_classes(L, classes); \ | ||
| lunatik_register_namespaces(L, namespaces); \ | ||
| return 1; \ | ||
| } \ |
There was a problem hiding this comment.
sorry, I know that I advised otherwise.. but now that we can see it implemented, I think it would be simpler to have just one API and to just add the class array to LUANATIK_NEWLIB.. I can help to update the other modules if needed.. it will be more alike the flags case.. we also need to keep in mind that we will need to update the README
There was a problem hiding this comment.
I'd also separate this API change in another commit.. before the acompress one
| local completion = require("completion") | ||
|
|
||
| --- @type COMPRESS | ||
| local COMPRESS = {} |
There was a problem hiding this comment.
I'm not sure we are using this pattern (with all caps) elsewhere.. it looked a bit odd to me.. but no strong feelings.. just a note..
| local status, comp = pcall(require, "crypto.comp") | ||
| if not status then | ||
| print("crypto.comp not available, skipping tests") | ||
| return | ||
| end |
There was a problem hiding this comment.
instead of doing this, we can register an empty library in this case.. we already do it on luaxdp if I'm not mistaken..
| u8 *outbuf; | ||
| size_t outbuf_len; |
There was a problem hiding this comment.
this is a very repetitive pattern, we use it on luadata (perhaps on other modules as well).. wondering if it wouldn't worth to have a lunatik_buffer_t or similar..
Fixes #307