diff mbox series

Lua: Log Lua error messages for external handlers

Message ID YfAgXED+BhsZg1qU@decadent.org.uk
State Changes Requested
Headers show
Series Lua: Log Lua error messages for external handlers | expand

Commit Message

Ben Hutchings Jan. 25, 2022, 4:07 p.m. UTC
Currently, any failure in initialisation of the swupdate_handlers
package results in a generic error message from SWUpdate.  This makes
it very difficult to debug initialisation failures.

Add the same error logging that is done in the case of embedded Lua
handlers.

Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
---
 corelib/lua_interface.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Stefano Babic Jan. 26, 2022, 3:17 p.m. UTC | #1
Hi Ben,

On 25.01.22 17:07, Ben Hutchings wrote:
> Currently, any failure in initialisation of the swupdate_handlers
> package results in a generic error message from SWUpdate.  This makes
> it very difficult to debug initialisation failures.
> 
> Add the same error logging that is done in the case of embedded Lua
> handlers.
> 
> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
> ---
>   corelib/lua_interface.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> index fe977c1..15b9e72 100644
> --- a/corelib/lua_interface.c
> +++ b/corelib/lua_interface.c
> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
>   #else
>   		if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
>   			INFO("No Lua handler(s) found.");
> +			TRACE("Lua exception:\n%s", lua_tostring(gL, -1));

Ok, but we to distinguish between real errors and not provided file. In 
most projects, it is foreseen to have an external file, but it is not 
provided leading to the "No Lua handler" - and this is not an error.

Your case seems thatyou provide swupdate_handler.lua, but there is an 
error to load the scripts. This should be detected from the case where 
no file is found.

Best regards,
Stefano Babic

> +			lua_pop(gL, 1);
>   			if (luaL_dostring(gL, "return package.path:gsub('?','swupdate_handlers'):gsub(';','\\0')") == 0) {
>   				const char *paths = lua_tostring(gL, -2);
>   				for (int i=lua_tonumber(gL, -1); i >= 0; i--) {
Ben Hutchings Jan. 26, 2022, 3:35 p.m. UTC | #2
Stefano Babic wrote:
> Hi Ben,
>
> On 25.01.22 17:07, Ben Hutchings wrote:
> > Currently, any failure in initialisation of the swupdate_handlers
> > package results in a generic error message from SWUpdate.  This makes
> > it very difficult to debug initialisation failures.
> >
> > Add the same error logging that is done in the case of embedded Lua
> > handlers.
> >
> > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
> > ---
> >   corelib/lua_interface.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> > index fe977c1..15b9e72 100644
> > --- a/corelib/lua_interface.c
> > +++ b/corelib/lua_interface.c
> > @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
> >   #else
> >               if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
> >                       INFO("No Lua handler(s) found.");
> > +                     TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
>
> Ok, but we to distinguish between real errors and not provided file. In
> most projects, it is foreseen to have an external file, but it is not
> provided leading to the "No Lua handler" - and this is not an error.
>
> Your case seems thatyou provide swupdate_handler.lua, but there is an
> error to load the scripts.

Right. The current behaviour makes it impossible to debug errors at load
time.

> This should be detected from the case where
> no file is found.

That's an interesting point. I don't think there is a way within the Lua API
to make that distinction.

Could we install an empty swupdate_handlers.lua if there is none already
installed, so that "file not found" shouldn't happen?

Ben.
Stefano Babic Jan. 26, 2022, 3:52 p.m. UTC | #3
Hi Ben,

On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote:
> Stefano Babic wrote:
>> Hi Ben,
>>
>> On 25.01.22 17:07, Ben Hutchings wrote:
>>> Currently, any failure in initialisation of the swupdate_handlers
>>> package results in a generic error message from SWUpdate.  This makes
>>> it very difficult to debug initialisation failures.
>>>
>>> Add the same error logging that is done in the case of embedded Lua
>>> handlers.
>>>
>>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
>>> ---
>>>    corelib/lua_interface.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
>>> index fe977c1..15b9e72 100644
>>> --- a/corelib/lua_interface.c
>>> +++ b/corelib/lua_interface.c
>>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
>>>    #else
>>>                if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
>>>                        INFO("No Lua handler(s) found.");
>>> +                     TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
>>
>> Ok, but we to distinguish between real errors and not provided file. In
>> most projects, it is foreseen to have an external file, but it is not
>> provided leading to the "No Lua handler" - and this is not an error.
>>
>> Your case seems thatyou provide swupdate_handler.lua, but there is an
>> error to load the scripts.
> 
> Right. The current behaviour makes it impossible to debug errors at load
> time.

I understand.

> 
>> This should be detected from the case where
>> no file is found.
> 
> That's an interesting point. I don't think there is a way within the Lua API
> to make that distinction.

When I looked at it, I do not find anything, too.

Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case, 
swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. 
And then you can consider that, if "No Lua handler(s) found." is raised 
but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and 
you can raise ERROR (instead of TRACE).

> 
> Could we install an empty swupdate_handlers.lua if there is none already
> installed, so that "file not found" shouldn't happen?

I cannot drive how users have integrated SWUpdate, and in most cases 
there is no swupdate_handler.lua at all, but it is open if a next 
version will added it. So this changes how SWUpdate is integrated and I 
am quite sure it will break a lot of projects. Check if my suggestion 
with CONFIG_EMBEDDED_LUA_HANDLER is ok for you, and I can integrate a 
patch like:

                        INFO("No Lua handler(s) found.");
+ if defined(CONFIG_EMBEDDED_LUA_HANDLER)
+                     ERROR("Lua exception:\n%s", lua_tostring(gL, -1));
+endif

Best regards,
Stefano
Christian Storm Jan. 27, 2022, 3:51 p.m. UTC | #4
Hi,

> > > > Currently, any failure in initialisation of the swupdate_handlers
> > > > package results in a generic error message from SWUpdate.  This makes
> > > > it very difficult to debug initialisation failures.
> > > > 
> > > > Add the same error logging that is done in the case of embedded Lua
> > > > handlers.
> > > > 
> > > > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
> > > > ---
> > > >    corelib/lua_interface.c | 2 ++
> > > >    1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> > > > index fe977c1..15b9e72 100644
> > > > --- a/corelib/lua_interface.c
> > > > +++ b/corelib/lua_interface.c
> > > > @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
> > > >    #else
> > > >                if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
> > > >                        INFO("No Lua handler(s) found.");
> > > > +                     TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
> > > 
> > > Ok, but we to distinguish between real errors and not provided file. In
> > > most projects, it is foreseen to have an external file, but it is not
> > > provided leading to the "No Lua handler" - and this is not an error.
> > > 
> > > Your case seems thatyou provide swupdate_handler.lua, but there is an
> > > error to load the scripts.
> > 
> > Right. The current behaviour makes it impossible to debug errors at load
> > time.
> 
> I understand.

Well, for me, LSPs and linters do a good job in telling me whether I did
something wrong in the Lua file, leaving the cases where interaction with
SWUpdate goes wrong. In this case, I do add (a variation of) the above
line though...

> > 
> > > This should be detected from the case where
> > > no file is found.
> > 
> > That's an interesting point. I don't think there is a way within the Lua API
> > to make that distinction.
> 
> When I looked at it, I do not find anything, too.

If you do require("module"), module gets searched for in package.path
which is initialized with $LUA_PATH or a compile-time default if absent.
So, you could iterate package.path to see if module can be found therein.
If not, the file is absent. Otherwise, there was an error in the file.

Another option is to place the file somewhere defined and use dofile() /
luaL_dofile(). Then, it's obvious whether the file is present or not.
The downside is that you have to define such a path and hence establish
a convention (probably compile-time configurable). What to do about
require("...")'s within this file then? They're still searched in
package.path and if they're not satisfied it also breaks ― even if
the file is present...


> Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case,
> swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself. And
> then you can consider that, if "No Lua handler(s) found." is raised but
> CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and you can
> raise ERROR (instead of TRACE).

Indeed. The only downside is that, while development, you need to relink
SWUpdate so to embed the new Lua script instead of just modifying the Lua
file and restarting SWUpdate.


> > Could we install an empty swupdate_handlers.lua if there is none already
> > installed, so that "file not found" shouldn't happen?

See dofile() / luaL_dofile() above.


> I cannot drive how users have integrated SWUpdate, and in most cases there is
> no swupdate_handler.lua at all, but it is open if a next version will added
> it. So this changes how SWUpdate is integrated and I am quite sure it will
> break a lot of projects. Check if my suggestion with
> CONFIG_EMBEDDED_LUA_HANDLER is ok for you, and I can integrate a patch like:
> 
>                        INFO("No Lua handler(s) found.");
> + if defined(CONFIG_EMBEDDED_LUA_HANDLER)
> +                     ERROR("Lua exception:\n%s", lua_tostring(gL, -1));
> +endif

This makes sense.



Kind regards,
   Christian
Ben Hutchings Feb. 7, 2022, 8 p.m. UTC | #5
Stefano Babic wrote:
> Hi Ben,
> 
> On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote:
> > Stefano Babic wrote:
> >> Hi Ben,
> >>
> >> On 25.01.22 17:07, Ben Hutchings wrote:
> >>> Currently, any failure in initialisation of the swupdate_handlers
> >>> package results in a generic error message from SWUpdate.  This makes
> >>> it very difficult to debug initialisation failures.
> >>>
> >>> Add the same error logging that is done in the case of embedded Lua
> >>> handlers.
> >>>
> >>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
> >>> ---
> >>>    corelib/lua_interface.c | 2 ++
> >>>    1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> >>> index fe977c1..15b9e72 100644
> >>> --- a/corelib/lua_interface.c
> >>> +++ b/corelib/lua_interface.c
> >>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
> >>>    #else
> >>>                if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
> >>>                        INFO("No Lua handler(s) found.");
> >>> +                     TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
> >>
> >> Ok, but we to distinguish between real errors and not provided file. In
> >> most projects, it is foreseen to have an external file, but it is not
> >> provided leading to the "No Lua handler" - and this is not an error.
> >>
> >> Your case seems thatyou provide swupdate_handler.lua, but there is an
> >> error to load the scripts.
> >
> > Right. The current behaviour makes it impossible to debug errors at load
> > time.
> 
> I understand.
> 
> >
> >> This should be detected from the case where
> >> no file is found.
> >
> > That's an interesting point. I don't think there is a way within the Lua API
> > to make that distinction.
> 
> When I looked at it, I do not find anything, too.
> 
> Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case,
> swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself.
> And then you can consider that, if "No Lua handler(s) found." is raised
> but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and
> you can raise ERROR (instead of TRACE).
[...]

I looked back at this and realised that I don't understand what
your objection is to this patch.

I understand that you don't want to see an error reported if
external Lua handlers are supported but not installed.  But this
patch doesn't do that.

Ben.
Stefano Babic Feb. 7, 2022, 8:21 p.m. UTC | #6
Hi Ben,

On 07.02.22 21:00, Ben HUTCHINGS (EXT) wrote:
> Stefano Babic wrote:
>> Hi Ben,
>>
>> On 26.01.22 16:35, Ben HUTCHINGS (EXT) wrote:
>>> Stefano Babic wrote:
>>>> Hi Ben,
>>>>
>>>> On 25.01.22 17:07, Ben Hutchings wrote:
>>>>> Currently, any failure in initialisation of the swupdate_handlers
>>>>> package results in a generic error message from SWUpdate.  This makes
>>>>> it very difficult to debug initialisation failures.
>>>>>
>>>>> Add the same error logging that is done in the case of embedded Lua
>>>>> handlers.
>>>>>
>>>>> Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com>
>>>>> ---
>>>>>     corelib/lua_interface.c | 2 ++
>>>>>     1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
>>>>> index fe977c1..15b9e72 100644
>>>>> --- a/corelib/lua_interface.c
>>>>> +++ b/corelib/lua_interface.c
>>>>> @@ -1238,6 +1238,8 @@ int lua_handlers_init(void)
>>>>>     #else
>>>>>                 if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
>>>>>                         INFO("No Lua handler(s) found.");
>>>>> +                     TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
>>>>
>>>> Ok, but we to distinguish between real errors and not provided file. In
>>>> most projects, it is foreseen to have an external file, but it is not
>>>> provided leading to the "No Lua handler" - and this is not an error.
>>>>
>>>> Your case seems thatyou provide swupdate_handler.lua, but there is an
>>>> error to load the scripts.
>>>
>>> Right. The current behaviour makes it impossible to debug errors at load
>>> time.
>>
>> I understand.
>>
>>>
>>>> This should be detected from the case where
>>>> no file is found.
>>>
>>> That's an interesting point. I don't think there is a way within the Lua API
>>> to make that distinction.
>>
>> When I looked at it, I do not find anything, too.
>>
>> Another solution for you is CONFIG_EMBEDDED_LUA_HANDLER. In this case,
>> swupdate_handler.lua is not loaded, but it is linked to SWUpdate itself.
>> And then you can consider that, if "No Lua handler(s) found." is raised
>> but CONFIG_EMBEDDED_LUA_HANDLER is set, this is absolutely an error and
>> you can raise ERROR (instead of TRACE).
> [...]
> 
> I looked back at this and realised that I don't understand what
> your objection is to this patch.
> 
> I understand that you don't want to see an error reported if
> external Lua handlers are supported but not installed.

Ys, more or less.

>  But this
> patch doesn't do that.

What I see applying the patch and dropping swupdate_handler.lua, that is 
one of frequent use case where Lua handler is just activated :

Without patch:
---------------

Swupdate v2021.11.0

Licensed under GPLv2. See source distribution for detailed copyright 
notices.

[INFO ] : SWUPDATE running :  [lua_handlers_init] : No Lua handler(s) found.
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/share/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/share/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/lib/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/lib/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/share/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/share/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 	./swupdate_handlers.lua


	==> no errors, Lua search path goes to trace,everything in SWUpdate's log.

With the patch:
----------------
[INFO ] : SWUPDATE running :  [lua_handlers_init] : No Lua handler(s) found.
[TRACE] : SWUPDATE running :  [lua_handlers_init] : Lua exception:
[string "require ("swupdate_handlers")"]:1: module 'swupdate_handlers' 
not found:
	no field package.preload['swupdate_handlers']
	no file '/usr/local/share/lua/5.2/swupdate_handlers.lua'
	no file '/usr/local/share/lua/5.2/swupdate_handlers/init.lua'
	no file '/usr/local/lib/lua/5.2/swupdate_handlers.lua'
	no file '/usr/local/lib/lua/5.2/swupdate_handlers/init.lua'
	no file '/usr/share/lua/5.2/swupdate_handlers.lua'
	no file '/usr/share/lua/5.2/swupdate_handlers/init.lua'
	no file './swupdate_handlers.lua'
	no file '/usr/local/lib/lua/5.2/swupdate_handlers.so'
	no file '/usr/lib/x86_64-linux-gnu/lua/5.2/swupdate_handlers.so'
	no file '/usr/lib/lua/5.2/swupdate_handlers.so'
	no file '/usr/local/lib/lua/5.2/loadall.so'
	no file './swupdate_handlers.so'

	==> all of them are duplicated because they are already in, and they 
are just printed on stdout, even if they are not (TRACE / ERRORS drop 
everything after \n).

[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/share/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/share/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/lib/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/local/lib/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/share/lua/5.2/swupdate_handlers.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 
/usr/share/lua/5.2/swupdate_handlers/init.lua
[TRACE] : SWUPDATE running :  [lua_handlers_init] : 	./swupdate_handlers.lua

Best regards,
Stefano
diff mbox series

Patch

diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
index fe977c1..15b9e72 100644
--- a/corelib/lua_interface.c
+++ b/corelib/lua_interface.c
@@ -1238,6 +1238,8 @@  int lua_handlers_init(void)
 #else
 		if ((ret = luaL_dostring(gL, "require (\"swupdate_handlers\")")) != 0) {
 			INFO("No Lua handler(s) found.");
+			TRACE("Lua exception:\n%s", lua_tostring(gL, -1));
+			lua_pop(gL, 1);
 			if (luaL_dostring(gL, "return package.path:gsub('?','swupdate_handlers'):gsub(';','\\0')") == 0) {
 				const char *paths = lua_tostring(gL, -2);
 				for (int i=lua_tonumber(gL, -1); i >= 0; i--) {