| Message ID | 6DD3F99F-2B1A-4849-9C50-99C135E8355A@siemens.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Lua: Load built-in handlers into the global state only | expand |
Hi Christian, On 09.06.24 21:26, 'Storm, Christian' via swupdate wrote: > Do not load binary-embedded or handlers from swupdate_handlers.lua > into a session Lua state. They've been loaded into the global Lua > state on SWUpdate startup and take precedence over same-named session > handlers anyway since with 28592bdd "Do not register multiple handlers > with same name", loading same-named (session) handlers is simply skipped. > > So, loading handlers distributed with SWUpdate into a session Lua state > is futile -- except for require() side effects but those shouldn't leak > into a Lua session state either. > Before checking the patch itself, I would like to discuss which are goals and side effects here. The idea under a "session" Lua state is to be able to "follow" the installation, as later Lua scripts can get information from results from previous scripts, including the embedded script. The big difference I see is for scripts asking to run "locally", as it was before, where a new Lua state is created, or global/per session. So before we needed a global Lua state to register handlers from swupdate_handlers.lua or binary. The question is if we need it today, I have thought mnostly that the global (gL in code) can be simply replaced by the per session state, but your patches are in the opposite direction. I have mostly left gL as check at the startup, else we are informed about an error in handlker just when an update is started and swupdate_handlers.lua is loaded. After saying this, I am not sure about your patch "Add registered handler to current Lua stack". Have I understood well and this allows to add a handler poersistently, that means it is recorded for later update ? If yes, an update is not stateless, and further updates depend on the result of previous ones, and this is not what I would like to have. Have I misunderstood something ? Best regards, Stefano > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > corelib/lua_interface.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index 03b74aed..d913b654 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -1525,10 +1525,12 @@ int lua_handlers_init(lua_State *L) > "External"; > #endif > int ret = -1; > + bool load_handlers = false; > > if (!L) { > gL = luaL_newstate(); > L = gL; > + load_handlers = true; > } > if (L) { > /* prime gL as LUA_TYPE_HANDLER */ > @@ -1538,19 +1540,24 @@ int lua_handlers_init(lua_State *L) > luaL_openlibs(L); > luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); > lua_pop(L, 1); /* remove unused copy left on stack */ > - /* try to load Lua handlers for the swupdate system */ > + > + if (load_handlers) { > + /* try to load handlers into global Lua state */ > #if defined(CONFIG_EMBEDDED_LUA_HANDLER) > - ret = (luaL_loadbuffer(L, EMBEDDED_LUA_SRC_START, EMBEDDED_LUA_SRC_END-EMBEDDED_LUA_SRC_START, "LuaHandler") || > - lua_pcall(L, 0, LUA_MULTRET, 0)); > + ret = (luaL_loadbuffer(L, EMBEDDED_LUA_SRC_START, > + EMBEDDED_LUA_SRC_END-EMBEDDED_LUA_SRC_START, > + "LuaHandler") || > + lua_pcall(L, 0, LUA_MULTRET, 0)); > #else > - ret = luaL_dostring(L, "require (\"swupdate_handlers\")"); > + ret = luaL_dostring(L, "require (\"swupdate_handlers\")"); > #endif > - if (ret != 0) { > - INFO("%s Lua handler(s) not found.", location); > - lua_report_exception(L); > - lua_pop(L, 1); > - } else { > - INFO("%s Lua handler(s) found and loaded.", location); > + if (ret != 0) { > + INFO("%s Lua handler(s) not found.", location); > + lua_report_exception(L); > + lua_pop(L, 1); > + } else { > + INFO("%s Lua handler(s) found and loaded.", location); > + } > } > } else { > WARN("Unable to register Lua context for callbacks");
Hi Stefano, >> Do not load binary-embedded or handlers from swupdate_handlers.lua >> into a session Lua state. They've been loaded into the global Lua >> state on SWUpdate startup and take precedence over same-named session >> handlers anyway since with 28592bdd "Do not register multiple handlers >> with same name", loading same-named (session) handlers is simply skipped. >> >> So, loading handlers distributed with SWUpdate into a session Lua state >> is futile -- except for require() side effects but those shouldn't leak >> into a Lua session state either. >> > > Before checking the patch itself, I would like to discuss which are > goals and side effects here. Makes sense :) > The idea under a "session" Lua state is to > be able to "follow" the installation, as later Lua scripts can get > information from results from previous scripts, including the embedded > script. Makes sense as well to not pollute the different Lua states. > The big difference I see is for scripts asking to run "locally", > as it was before, where a new Lua state is created, or global/per session. > > So before we needed a global Lua state to register handlers from > swupdate_handlers.lua or binary. The question is if we need it today, I > have thought mnostly that the global (gL in code) can be simply replaced > by the per session state, but your patches are in the opposite direction. Well, to some extent. Current HEAD loads handlers into the Lua session state that is primed for being a Lua handler (session) state only after having done require("swupdate") primed as embedded script, so loading handlers into this session state fails. This is addressed by "[PATCH] Lua: Fix Lua session handlers". With that fixed, however, these handlers (binary-embedded or from swupdate_handlers.lua) have already been loaded + registered into gL on SWUpdate startup and trying to register a (session'ed) handler having the same name as one that's already registered (in gL) is simply skipped. Calling such a handler always runs the one from/registered to gL. Hence, it doesn't make sense to load binary-embedded handlers or handlers from swupdate_handlers.lua into the session Lua state as they will not be registered anyway. This is addressed by "[PATCH] Lua: Load built-in handlers into the global state only". [This is what goes into the "opposite direction", bear with me until TL;DR] "[PATCH] Lua: Add registered handler to current Lua stack" just reflects the list of loaded handlers into the current Lua stack and is a consequence of the former two patches. TL;DR: To actually "sessionize" handlers (binary-embedded or from swupdate_handlers.lua), they shouldn't be loaded + registered into gL on SWUpdate startup. You still need "[PATCH] Lua: Fix Lua session handlers" so that the session Lua state is able to load + register handlers. And you would like to have "[PATCH] Lua: Add registered handler to current Lua stack" so that later Lua scripts know about the list of actually installed (Lua) handlers within the session and not just the global ones (then containing just the C handlers list). > I have mostly left gL as check at the startup, else we are > informed about an error in handlker just when an update is started and > swupdate_handlers.lua is loaded. ... which is fail early. Instead of gL we could use some disposable Lua stack while early SWUpdate startup to load the Lua handlers (binary-embedded or from swupdate_handlers.lua) to catch apparent load errors. Then, as outlined above, handlers could be loaded + registered in the session Lua state, scoped. Note that just loading doesn't catch handler run-time errors with Lua being a dynamic language but that's the best we can do to fail early. Question is if that's worth the effort since you need to test the update flow anyway to detect run-time errors. > After saying this, I am not sure about your patch "Add registered > handler to current Lua stack". Have I understood well and this allows to > add a handler poersistently, that means it is recorded for later update > ? If yes, an update is not stateless, and further updates depend on the > result of previous ones, and this is not what I would like to have. Have > I misunderstood something ? I hope I have clarified these questions in my above elaborations? Kind regards, Christian
Hi Christian, On 11.06.24 12:14, 'Storm, Christian' via swupdate wrote: > Hi Stefano, > >>> Do not load binary-embedded or handlers from swupdate_handlers.lua >>> into a session Lua state. They've been loaded into the global Lua >>> state on SWUpdate startup and take precedence over same-named session >>> handlers anyway since with 28592bdd "Do not register multiple handlers >>> with same name", loading same-named (session) handlers is simply skipped. >>> >>> So, loading handlers distributed with SWUpdate into a session Lua state >>> is futile -- except for require() side effects but those shouldn't leak >>> into a Lua session state either. >>> >> >> Before checking the patch itself, I would like to discuss which are >> goals and side effects here. > > Makes sense :) > > >> The idea under a "session" Lua state is to >> be able to "follow" the installation, as later Lua scripts can get >> information from results from previous scripts, including the embedded >> script. > > Makes sense as well to not pollute the different Lua states. > > >> The big difference I see is for scripts asking to run "locally", >> as it was before, where a new Lua state is created, or global/per session. >> >> So before we needed a global Lua state to register handlers from >> swupdate_handlers.lua or binary. The question is if we need it today, I >> have thought mnostly that the global (gL in code) can be simply replaced >> by the per session state, but your patches are in the opposite direction. > > Well, to some extent. Current HEAD loads handlers into the Lua session state that is primed for being a Lua handler (session) state only after having done require("swupdate") primed as embedded script, so loading handlers into this session state fails. This is addressed by "[PATCH] Lua: Fix Lua session handlers". Ok, got it. > > With that fixed, however, these handlers (binary-embedded or from swupdate_handlers.lua) have already been loaded + registered into gL on SWUpdate startup and trying to register a (session'ed) handler having the same name as one that's already registered (in gL) is simply skipped. Correct, I explicitly avoid this. > Calling such a handler always runs the one from/registered to gL. Hence, it doesn't make sense to load binary-embedded handlers or handlers from swupdate_handlers.lua into the session Lua state as they will not be registered anyway. This is addressed by "[PATCH] Lua: Load built-in handlers into the global state only". [This is what goes into the "opposite direction", bear with me until TL;DR] Ok > > "[PATCH] Lua: Add registered handler to current Lua stack" just reflects the list of loaded handlers into the current Lua stack and is a consequence of the former two patches. > > TL;DR: To actually "sessionize" handlers (binary-embedded or from swupdate_handlers.lua), they shouldn't be loaded + registered into gL on SWUpdate startup. Right, it was just a commodity to check handler's syntax at startup. > You still need "[PATCH] Lua: Fix Lua session handlers" so that the session Lua state is able to load + register handlers. And you would like to have "[PATCH] Lua: Add registered handler to current Lua stack" so that later Lua scripts know about the list of actually installed (Lua) handlers within the session and not just the global ones (then containing just the C handlers list). > > >> I have mostly left gL as check at the startup, else we are >> informed about an error in handlker just when an update is started and >> swupdate_handlers.lua is loaded. > > ... which is fail early. Instead of gL we could use some disposable Lua stack while early SWUpdate startup to load the Lua handlers (binary-embedded or from swupdate_handlers.lua) to catch apparent load errors. Makes sense. >Then, as outlined above, handlers could be loaded + registered in the session Lua state, scoped. > > Note that just loading doesn't catch handler run-time errors with Lua being a dynamic language but that's the best we can do to fail early. Sure,but this is out of scope. We have just checked that handlers can be loaded in previous versions, and this should be maintained. If the handler works, this is tested during an update. > Question is if that's worth the effort since you need to test the update flow anyway to detect run-time errors. No, it isn't worth. > > >> After saying this, I am not sure about your patch "Add registered >> handler to current Lua stack". Have I understood well and this allows to >> add a handler poersistently, that means it is recorded for later update >> ? If yes, an update is not stateless, and further updates depend on the >> result of previous ones, and this is not what I would like to have. Have >> I misunderstood something ? > > I hope I have clarified these questions in my above elaborations? Thanks for clarification. Best regards, Stefano > > > Kind regards, > Christian >
Hi, >>>> Do not load binary-embedded or handlers from swupdate_handlers.lua >>>> into a session Lua state. They've been loaded into the global Lua >>>> state on SWUpdate startup and take precedence over same-named session >>>> handlers anyway since with 28592bdd "Do not register multiple handlers >>>> with same name", loading same-named (session) handlers is simply skipped. >>>> >>>> So, loading handlers distributed with SWUpdate into a session Lua state >>>> is futile -- except for require() side effects but those shouldn't leak >>>> into a Lua session state either. >>>> >>> >>> Before checking the patch itself, I would like to discuss which are >>> goals and side effects here. >> >> Makes sense :) >> >> >>> The idea under a "session" Lua state is to >>> be able to "follow" the installation, as later Lua scripts can get >>> information from results from previous scripts, including the embedded >>> script. >> >> Makes sense as well to not pollute the different Lua states. >> >> >>> The big difference I see is for scripts asking to run "locally", >>> as it was before, where a new Lua state is created, or global/per session. >>> >>> So before we needed a global Lua state to register handlers from >>> swupdate_handlers.lua or binary. The question is if we need it today, I >>> have thought mnostly that the global (gL in code) can be simply replaced >>> by the per session state, but your patches are in the opposite direction. >> >> Well, to some extent. Current HEAD loads handlers into the Lua session state that is primed for being a Lua handler (session) state only after having done require("swupdate") primed as embedded script, so loading handlers into this session state fails. This is addressed by "[PATCH] Lua: Fix Lua session handlers". > > Ok, got it. > >> >> With that fixed, however, these handlers (binary-embedded or from swupdate_handlers.lua) have already been loaded + registered into gL on SWUpdate startup and trying to register a (session'ed) handler having the same name as one that's already registered (in gL) is simply skipped. > > Correct, I explicitly avoid this. > >> Calling such a handler always runs the one from/registered to gL. Hence, it doesn't make sense to load binary-embedded handlers or handlers from swupdate_handlers.lua into the session Lua state as they will not be registered anyway. This is addressed by "[PATCH] Lua: Load built-in handlers into the global state only". [This is what goes into the "opposite direction", bear with me until TL;DR] > > Ok > > >> >> "[PATCH] Lua: Add registered handler to current Lua stack" just reflects the list of loaded handlers into the current Lua stack and is a consequence of the former two patches. >> >> TL;DR: To actually "sessionize" handlers (binary-embedded or from swupdate_handlers.lua), they shouldn't be loaded + registered into gL on SWUpdate startup. > > Right, it was just a commodity to check handler's syntax at startup. > >> You still need "[PATCH] Lua: Fix Lua session handlers" so that the session Lua state is able to load + register handlers. And you would like to have "[PATCH] Lua: Add registered handler to current Lua stack" so that later Lua scripts know about the list of actually installed (Lua) handlers within the session and not just the global ones (then containing just the C handlers list). Instead of this patch, I just sent 3 new patches to sessionize Lua handlers, so would [PATCH] Lua: Fix Lua session handlers [PATCH] Lua: Add registered handler to current Lua stack + new [PATCH] Lua: Rename lua_init() to lua_session_init() new [PATCH] Lua: Sessionize Lua Handlers new [PATCH] handler: Print registered session & global handlers make more sense in the right direction? Kind regards, Christian
Hi Christian, On 13.06.24 18:18, 'Storm, Christian' via swupdate wrote: > Hi, > > >>>>> Do not load binary-embedded or handlers from swupdate_handlers.lua >>>>> into a session Lua state. They've been loaded into the global Lua >>>>> state on SWUpdate startup and take precedence over same-named session >>>>> handlers anyway since with 28592bdd "Do not register multiple handlers >>>>> with same name", loading same-named (session) handlers is simply skipped. >>>>> >>>>> So, loading handlers distributed with SWUpdate into a session Lua state >>>>> is futile -- except for require() side effects but those shouldn't leak >>>>> into a Lua session state either. >>>>> >>>> >>>> Before checking the patch itself, I would like to discuss which are >>>> goals and side effects here. >>> >>> Makes sense :) >>> >>> >>>> The idea under a "session" Lua state is to >>>> be able to "follow" the installation, as later Lua scripts can get >>>> information from results from previous scripts, including the embedded >>>> script. >>> >>> Makes sense as well to not pollute the different Lua states. >>> >>> >>>> The big difference I see is for scripts asking to run "locally", >>>> as it was before, where a new Lua state is created, or global/per session. >>>> >>>> So before we needed a global Lua state to register handlers from >>>> swupdate_handlers.lua or binary. The question is if we need it today, I >>>> have thought mnostly that the global (gL in code) can be simply replaced >>>> by the per session state, but your patches are in the opposite direction. >>> >>> Well, to some extent. Current HEAD loads handlers into the Lua session state that is primed for being a Lua handler (session) state only after having done require("swupdate") primed as embedded script, so loading handlers into this session state fails. This is addressed by "[PATCH] Lua: Fix Lua session handlers". >> >> Ok, got it. >> >>> >>> With that fixed, however, these handlers (binary-embedded or from swupdate_handlers.lua) have already been loaded + registered into gL on SWUpdate startup and trying to register a (session'ed) handler having the same name as one that's already registered (in gL) is simply skipped. >> >> Correct, I explicitly avoid this. >> >>> Calling such a handler always runs the one from/registered to gL. Hence, it doesn't make sense to load binary-embedded handlers or handlers from swupdate_handlers.lua into the session Lua state as they will not be registered anyway. This is addressed by "[PATCH] Lua: Load built-in handlers into the global state only". [This is what goes into the "opposite direction", bear with me until TL;DR] >> >> Ok >> >> >>> >>> "[PATCH] Lua: Add registered handler to current Lua stack" just reflects the list of loaded handlers into the current Lua stack and is a consequence of the former two patches. >>> >>> TL;DR: To actually "sessionize" handlers (binary-embedded or from swupdate_handlers.lua), they shouldn't be loaded + registered into gL on SWUpdate startup. >> >> Right, it was just a commodity to check handler's syntax at startup. >> >>> You still need "[PATCH] Lua: Fix Lua session handlers" so that the session Lua state is able to load + register handlers. And you would like to have "[PATCH] Lua: Add registered handler to current Lua stack" so that later Lua scripts know about the list of actually installed (Lua) handlers within the session and not just the global ones (then containing just the C handlers list). > > > Instead of this patch, I just sent 3 new patches to sessionize Lua handlers, so would > > [PATCH] Lua: Fix Lua session handlers > [PATCH] Lua: Add registered handler to current Lua stack > + > new [PATCH] Lua: Rename lua_init() to lua_session_init() > new [PATCH] Lua: Sessionize Lua Handlers > new [PATCH] handler: Print registered session & global handlers > > make more sense in the right direction? For me yes - do not think me twice, I am open to different approaches. But IMHO a global Lua state does not make sense anymore, and having a per update Lua session brings more advantages like building an own logic completely outside the core, and each piece of Lua code can know what previous Lua scripts have done before. Best regards, Stefano > > > Kind regards, > Christian >
Hi, >>>>>> Do not load binary-embedded or handlers from swupdate_handlers.lua >>>>>> into a session Lua state. They've been loaded into the global Lua >>>>>> state on SWUpdate startup and take precedence over same-named session >>>>>> handlers anyway since with 28592bdd "Do not register multiple handlers >>>>>> with same name", loading same-named (session) handlers is simply skipped. >>>>>> >>>>>> So, loading handlers distributed with SWUpdate into a session Lua state >>>>>> is futile -- except for require() side effects but those shouldn't leak >>>>>> into a Lua session state either. >>>>>> >>>>> >>>>> Before checking the patch itself, I would like to discuss which are >>>>> goals and side effects here. >>>> >>>> Makes sense :) >>>> >>>> >>>>> The idea under a "session" Lua state is to >>>>> be able to "follow" the installation, as later Lua scripts can get >>>>> information from results from previous scripts, including the embedded >>>>> script. >>>> >>>> Makes sense as well to not pollute the different Lua states. >>>> >>>> >>>>> The big difference I see is for scripts asking to run "locally", >>>>> as it was before, where a new Lua state is created, or global/per session. >>>>> >>>>> So before we needed a global Lua state to register handlers from >>>>> swupdate_handlers.lua or binary. The question is if we need it today, I >>>>> have thought mnostly that the global (gL in code) can be simply replaced >>>>> by the per session state, but your patches are in the opposite direction. >>>> >>>> Well, to some extent. Current HEAD loads handlers into the Lua session state that is primed for being a Lua handler (session) state only after having done require("swupdate") primed as embedded script, so loading handlers into this session state fails. This is addressed by "[PATCH] Lua: Fix Lua session handlers". >>> >>> Ok, got it. >>> >>>> >>>> With that fixed, however, these handlers (binary-embedded or from swupdate_handlers.lua) have already been loaded + registered into gL on SWUpdate startup and trying to register a (session'ed) handler having the same name as one that's already registered (in gL) is simply skipped. >>> >>> Correct, I explicitly avoid this. >>> >>>> Calling such a handler always runs the one from/registered to gL. Hence, it doesn't make sense to load binary-embedded handlers or handlers from swupdate_handlers.lua into the session Lua state as they will not be registered anyway. This is addressed by "[PATCH] Lua: Load built-in handlers into the global state only". [This is what goes into the "opposite direction", bear with me until TL;DR] >>> >>> Ok >>> >>> >>>> >>>> "[PATCH] Lua: Add registered handler to current Lua stack" just reflects the list of loaded handlers into the current Lua stack and is a consequence of the former two patches. >>>> >>>> TL;DR: To actually "sessionize" handlers (binary-embedded or from swupdate_handlers.lua), they shouldn't be loaded + registered into gL on SWUpdate startup. >>> >>> Right, it was just a commodity to check handler's syntax at startup. >>> >>>> You still need "[PATCH] Lua: Fix Lua session handlers" so that the session Lua state is able to load + register handlers. And you would like to have "[PATCH] Lua: Add registered handler to current Lua stack" so that later Lua scripts know about the list of actually installed (Lua) handlers within the session and not just the global ones (then containing just the C handlers list). >> >> >> Instead of this patch, I just sent 3 new patches to sessionize Lua handlers, so would >> >> [PATCH] Lua: Fix Lua session handlers >> [PATCH] Lua: Add registered handler to current Lua stack >> + >> new [PATCH] Lua: Rename lua_init() to lua_session_init() >> new [PATCH] Lua: Sessionize Lua Handlers >> new [PATCH] handler: Print registered session & global handlers >> >> make more sense in the right direction? > > For me yes - do not think me twice, I am open to different approaches. > But IMHO a global Lua state does not make sense anymore, and having a > per update Lua session brings more advantages like building an own logic > completely outside the core, and each piece of Lua code can know what > previous Lua scripts have done before. Fine with me, I'll send a patch removing the global Lua state. Kind regards, Christian
diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index 03b74aed..d913b654 100644 --- a/corelib/lua_interface.c +++ b/corelib/lua_interface.c @@ -1525,10 +1525,12 @@ int lua_handlers_init(lua_State *L) "External"; #endif int ret = -1; + bool load_handlers = false; if (!L) { gL = luaL_newstate(); L = gL; + load_handlers = true; } if (L) { /* prime gL as LUA_TYPE_HANDLER */ @@ -1538,19 +1540,24 @@ int lua_handlers_init(lua_State *L) luaL_openlibs(L); luaL_requiref(L, "swupdate", luaopen_swupdate, 1 ); lua_pop(L, 1); /* remove unused copy left on stack */ - /* try to load Lua handlers for the swupdate system */ + + if (load_handlers) { + /* try to load handlers into global Lua state */ #if defined(CONFIG_EMBEDDED_LUA_HANDLER) - ret = (luaL_loadbuffer(L, EMBEDDED_LUA_SRC_START, EMBEDDED_LUA_SRC_END-EMBEDDED_LUA_SRC_START, "LuaHandler") || - lua_pcall(L, 0, LUA_MULTRET, 0)); + ret = (luaL_loadbuffer(L, EMBEDDED_LUA_SRC_START, + EMBEDDED_LUA_SRC_END-EMBEDDED_LUA_SRC_START, + "LuaHandler") || + lua_pcall(L, 0, LUA_MULTRET, 0)); #else - ret = luaL_dostring(L, "require (\"swupdate_handlers\")"); + ret = luaL_dostring(L, "require (\"swupdate_handlers\")"); #endif - if (ret != 0) { - INFO("%s Lua handler(s) not found.", location); - lua_report_exception(L); - lua_pop(L, 1); - } else { - INFO("%s Lua handler(s) found and loaded.", location); + if (ret != 0) { + INFO("%s Lua handler(s) not found.", location); + lua_report_exception(L); + lua_pop(L, 1); + } else { + INFO("%s Lua handler(s) found and loaded.", location); + } } } else { WARN("Unable to register Lua context for callbacks");
Do not load binary-embedded or handlers from swupdate_handlers.lua into a session Lua state. They've been loaded into the global Lua state on SWUpdate startup and take precedence over same-named session handlers anyway since with 28592bdd "Do not register multiple handlers with same name", loading same-named (session) handlers is simply skipped. So, loading handlers distributed with SWUpdate into a session Lua state is futile -- except for require() side effects but those shouldn't leak into a Lua session state either. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- corelib/lua_interface.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)