| Message ID | 1516199996-5855-1-git-send-email-stefan@herbrechtsmeier.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Lua: Export SWUpdate bootenv get and set functions to Lua scripts | expand |
Hi Stefan, On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > --- > > corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index 25d84e6..f370474 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -16,6 +16,7 @@ > #include "lua_util.h" > #include "util.h" > #include "handler.h" > +#include "bootloader.h" > > #define LUA_TYPE_PEMBSCR 1 > #define LUA_TYPE_HANDLER 2 > @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { > return 0; > } > > +static int l_get_bootenv(lua_State *L) { > + const char *name = luaL_checkstring(L, 1); > + char *value = NULL; > + > + if (strlen(name)) > + value = bootloader_env_get(name); > + lua_pop(L, 1); > + > + lua_pushstring(L, value); > + free(value); > + > + return 1; > +} > + > +static int l_set_bootenv(lua_State *L) { > + const char *name = luaL_checkstring(L, 1); > + const char *value = luaL_checkstring(L, 2); > + > + if (strlen(name)) { > + if (strlen(value)) > + bootloader_env_set(name, value); > + else > + bootloader_env_unset(name); > + } > + lua_pop(L, 2); > + > + return 0; > +} > + > #ifdef CONFIG_HANDLER_IN_LUA > static int l_get_tmpdir(lua_State *L) > { > @@ -664,6 +694,8 @@ static const luaL_Reg l_swupdate[] = { > { "error", l_error }, > { "trace", l_trace }, > { "info", l_info }, > + { "get_bootenv", l_get_bootenv }, > + { "set_bootenv", l_set_bootenv }, > { NULL, NULL } > }; > > A set() could be very dangerous. I can understand that it could be helpful, but it is very difficult to track when something is going wronf and could break the system. Think about this scenario: a LUA (jandler, script) is accessing to the environment and changes. Everything fine, after that the next handler finds an error and aborts the installation. The system remains with the environment updated by the LUA actor, not with the original - system does not maybe boot anymore. This is the reason why u-boot is set at the end of the update: all handlers ran, and we are sure that upgrading the environment is the last step. Even if it fails, we have still the old environment. But if a LUA script can do this, everything happens. I agree that someone (I read from ML) has the good idea to use fw_setnev inside shell scripts - another way to try to brick the device. IMHO this looks dangerous.... Note: l_get_bootenv() can be added without issues, of course. Best regards, Stefano
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Mittwoch, 17. Januar 2018 15:48 > An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Cc: Herbrechtsmeier Dr.-Ing. , Stefan > <Stefan.Herbrechtsmeier@weidmueller.com> > Betreff: Re: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get and set > functions to Lua scripts > > On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: > > From: Stefan Herbrechtsmeier > <stefan.herbrechtsmeier@weidmueller.com> > > > > Signed-off-by: Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier@weidmueller.com> > > --- > > > > corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index > > 25d84e6..f370474 100644 > > --- a/corelib/lua_interface.c > > +++ b/corelib/lua_interface.c > > @@ -16,6 +16,7 @@ > > #include "lua_util.h" > > #include "util.h" > > #include "handler.h" > > +#include "bootloader.h" > > > > #define LUA_TYPE_PEMBSCR 1 > > #define LUA_TYPE_HANDLER 2 > > @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { > > return 0; > > } > > > > +static int l_get_bootenv(lua_State *L) { > > + const char *name = luaL_checkstring(L, 1); > > + char *value = NULL; > > + > > + if (strlen(name)) > > + value = bootloader_env_get(name); > > + lua_pop(L, 1); > > + > > + lua_pushstring(L, value); > > + free(value); > > + > > + return 1; > > +} > > + > > +static int l_set_bootenv(lua_State *L) { > > + const char *name = luaL_checkstring(L, 1); > > + const char *value = luaL_checkstring(L, 2); > > + > > + if (strlen(name)) { > > + if (strlen(value)) > > + bootloader_env_set(name, value); > > + else > > + bootloader_env_unset(name); > > + } > > + lua_pop(L, 2); > > + > > + return 0; > > +} > > + > > #ifdef CONFIG_HANDLER_IN_LUA > > static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ static > > const luaL_Reg l_swupdate[] = { > > { "error", l_error }, > > { "trace", l_trace }, > > { "info", l_info }, > > + { "get_bootenv", l_get_bootenv }, > > + { "set_bootenv", l_set_bootenv }, > > { NULL, NULL } > > }; > > > > > > A set() could be very dangerous. I can understand that it could be helpful, > but it is very difficult to track when something is going wronf and could break > the system. > > Think about this scenario: a LUA (jandler, script) is accessing to the > environment and changes. Everything fine, after that the next handler finds > an error and aborts the installation. The system remains with the > environment updated by the LUA actor, not with the original - system does > not maybe boot anymore. > > This is the reason why u-boot is set at the end of the update: all handlers ran, > and we are sure that upgrading the environment is the last step. Even if it > fails, we have still the old environment. > > But if a LUA script can do this, everything happens. > > I agree that someone (I read from ML) has the good idea to use fw_setnev > inside shell scripts - another way to try to brick the device. > > IMHO this looks dangerous.... I understand your concerns but the "-e" parameter has also the potential to break your system if you don't restart the system between two updates. Anyway let's keep the risk low if other solution exists. Alternative I can add support for a Lua hook script to the bootloader environment entries to change the value at parse time and execute the changes at the end of the update. But therefore I need to extend and temporally misuse the img_type to pass the name and value to a Lua handler. > Note: l_get_bootenv() can be added without issues, of course. I will resend the patch without the set function. Best regards Stefan Herbrechtsmeier Software Developer Embedded Systems Weidmüller – Your partner in Industrial Connectivity We look forward to sharing ideas with you – Let’s connect. Weidmueller Interface GmbH & Co. KG Klingenbergstraße 16, 32758 Detmold, Germany Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
Hi Stefan, On 17/01/2018 16:38, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Mittwoch, 17. Januar 2018 15:48 >> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >> <Stefan.Herbrechtsmeier@weidmueller.com> >> Betreff: Re: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get and set >> functions to Lua scripts >> >> On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: >>> From: Stefan Herbrechtsmeier >> <stefan.herbrechtsmeier@weidmueller.com> >>> >>> Signed-off-by: Stefan Herbrechtsmeier >>> <stefan.herbrechtsmeier@weidmueller.com> >>> --- >>> >>> corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ >>> 1 file changed, 32 insertions(+) >>> >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index >>> 25d84e6..f370474 100644 >>> --- a/corelib/lua_interface.c >>> +++ b/corelib/lua_interface.c >>> @@ -16,6 +16,7 @@ >>> #include "lua_util.h" >>> #include "util.h" >>> #include "handler.h" >>> +#include "bootloader.h" >>> >>> #define LUA_TYPE_PEMBSCR 1 >>> #define LUA_TYPE_HANDLER 2 >>> @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { >>> return 0; >>> } >>> >>> +static int l_get_bootenv(lua_State *L) { >>> + const char *name = luaL_checkstring(L, 1); >>> + char *value = NULL; >>> + >>> + if (strlen(name)) >>> + value = bootloader_env_get(name); >>> + lua_pop(L, 1); >>> + >>> + lua_pushstring(L, value); >>> + free(value); >>> + >>> + return 1; >>> +} >>> + >>> +static int l_set_bootenv(lua_State *L) { >>> + const char *name = luaL_checkstring(L, 1); >>> + const char *value = luaL_checkstring(L, 2); >>> + >>> + if (strlen(name)) { >>> + if (strlen(value)) >>> + bootloader_env_set(name, value); >>> + else >>> + bootloader_env_unset(name); >>> + } >>> + lua_pop(L, 2); >>> + >>> + return 0; >>> +} >>> + >>> #ifdef CONFIG_HANDLER_IN_LUA >>> static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ static >>> const luaL_Reg l_swupdate[] = { >>> { "error", l_error }, >>> { "trace", l_trace }, >>> { "info", l_info }, >>> + { "get_bootenv", l_get_bootenv }, >>> + { "set_bootenv", l_set_bootenv }, >>> { NULL, NULL } >>> }; >>> >>> >> >> A set() could be very dangerous. I can understand that it could be helpful, >> but it is very difficult to track when something is going wronf and could break >> the system. >> >> Think about this scenario: a LUA (jandler, script) is accessing to the >> environment and changes. Everything fine, after that the next handler finds >> an error and aborts the installation. The system remains with the >> environment updated by the LUA actor, not with the original - system does >> not maybe boot anymore. >> >> This is the reason why u-boot is set at the end of the update: all handlers ran, >> and we are sure that upgrading the environment is the last step. Even if it >> fails, we have still the old environment. >> >> But if a LUA script can do this, everything happens. >> >> I agree that someone (I read from ML) has the good idea to use fw_setnev >> inside shell scripts - another way to try to brick the device. >> >> IMHO this looks dangerous.... > > I understand your concerns but the "-e" parameter has also the potential to break your system if you don't restart the system between two updates. Not exactly - you are talking about a misuse. SWUpdate is more a framework, and if the update concept is wrong, there is nothing that can be done. You example applies just to double-copy, and the designer should take care that a successful update requires a restart. If the design is correct, there is no risk. > Anyway let's keep the risk low if other solution exists. > > Alternative I can add support for a Lua hook script to the bootloader environment entries to change the value at parse time and execute the changes at the end of the update. Maybe we are thinking the same. I thought to have a "set_bootenv" that handles the bootloader list. The update will be done at the end as usual. Is this what you mind ? > But therefore I need to extend and temporally misuse the img_type to pass the name and value to a Lua handler. > mmmhh...get_bootenv in LUA returns the environment, and set_bootenv(var, value) will change just a variable in the list. Not sure whhat you plan to do. >> Note: l_get_bootenv() can be added without issues, of course. > > I will resend the patch without the set function. Ok, fine. Best regards, Stefano
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Mittwoch, 17. Januar 2018 16:59 > An: Herbrechtsmeier Dr.-Ing. , Stefan > <Stefan.Herbrechtsmeier@weidmueller.com>; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get > and set functions to Lua scripts > > On 17/01/2018 16:38, Stefan.Herbrechtsmeier@weidmueller.com wrote: > > Hi Stefano, > > > >> -----Ursprüngliche Nachricht----- > >> Von: Stefano Babic [mailto:sbabic@denx.de] > >> Gesendet: Mittwoch, 17. Januar 2018 15:48 > >> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >> Cc: Herbrechtsmeier Dr.-Ing. , Stefan > >> <Stefan.Herbrechtsmeier@weidmueller.com> > >> Betreff: Re: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get and > >> set functions to Lua scripts > >> > >> On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: > >>> From: Stefan Herbrechtsmeier > >> <stefan.herbrechtsmeier@weidmueller.com> > >>> > >>> Signed-off-by: Stefan Herbrechtsmeier > >>> <stefan.herbrechtsmeier@weidmueller.com> > >>> --- > >>> > >>> corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ > >>> 1 file changed, 32 insertions(+) > >>> > >>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index > >>> 25d84e6..f370474 100644 > >>> --- a/corelib/lua_interface.c > >>> +++ b/corelib/lua_interface.c > >>> @@ -16,6 +16,7 @@ > >>> #include "lua_util.h" > >>> #include "util.h" > >>> #include "handler.h" > >>> +#include "bootloader.h" > >>> > >>> #define LUA_TYPE_PEMBSCR 1 > >>> #define LUA_TYPE_HANDLER 2 > >>> @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { > >>> return 0; > >>> } > >>> > >>> +static int l_get_bootenv(lua_State *L) { > >>> + const char *name = luaL_checkstring(L, 1); > >>> + char *value = NULL; > >>> + > >>> + if (strlen(name)) > >>> + value = bootloader_env_get(name); > >>> + lua_pop(L, 1); > >>> + > >>> + lua_pushstring(L, value); > >>> + free(value); > >>> + > >>> + return 1; > >>> +} > >>> + > >>> +static int l_set_bootenv(lua_State *L) { > >>> + const char *name = luaL_checkstring(L, 1); > >>> + const char *value = luaL_checkstring(L, 2); > >>> + > >>> + if (strlen(name)) { > >>> + if (strlen(value)) > >>> + bootloader_env_set(name, value); > >>> + else > >>> + bootloader_env_unset(name); > >>> + } > >>> + lua_pop(L, 2); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> #ifdef CONFIG_HANDLER_IN_LUA > >>> static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ static > >>> const luaL_Reg l_swupdate[] = { > >>> { "error", l_error }, > >>> { "trace", l_trace }, > >>> { "info", l_info }, > >>> + { "get_bootenv", l_get_bootenv }, > >>> + { "set_bootenv", l_set_bootenv }, > >>> { NULL, NULL } > >>> }; > >>> > >>> > >> > >> A set() could be very dangerous. I can understand that it could be > >> helpful, but it is very difficult to track when something is going > >> wronf and could break the system. > >> > >> Think about this scenario: a LUA (jandler, script) is accessing to > >> the environment and changes. Everything fine, after that the next > >> handler finds an error and aborts the installation. The system > >> remains with the environment updated by the LUA actor, not with the > >> original - system does not maybe boot anymore. > >> > >> This is the reason why u-boot is set at the end of the update: all > >> handlers ran, and we are sure that upgrading the environment is the > >> last step. Even if it fails, we have still the old environment. > >> > >> But if a LUA script can do this, everything happens. > >> > >> I agree that someone (I read from ML) has the good idea to use > >> fw_setnev inside shell scripts - another way to try to brick the device. > >> > >> IMHO this looks dangerous.... > > > > I understand your concerns but the "-e" parameter has also the potential > to break your system if you don't restart the system between two updates. > > Not exactly - you are talking about a misuse. SWUpdate is more a framework, > and if the update concept is wrong, there is nothing that can be done. You > example applies just to double-copy, and the designer should take care that > a successful update requires a restart. If the design is correct, there is no risk. > > > Anyway let's keep the risk low if other solution exists. > > > > Alternative I can add support for a Lua hook script to the bootloader > environment entries to change the value at parse time and execute the > changes at the end of the update. > > Maybe we are thinking the same. I thought to have a "set_bootenv" that > handles the bootloader list. The update will be done at the end as usual. Is > this what you mind ? I mean a hook to an embedded-script function in the sw-description: bootenv: ( { name = "vram"; value = "4M"; hook = "my_lua_script"; } ); Do you mean a set_bootenv function which only updates the bootloader environment dictionary? How will you pass the bootloader dictionary to this function? > > But therefore I need to extend and temporally misuse the img_type to > pass the name and value to a Lua handler. > > > > mmmhh...get_bootenv in LUA returns the environment, and > set_bootenv(var, > value) will change just a variable in the list. Not sure whhat you plan to do. I need a function to read the current bootloader value. Therefore the get_bootenv should return the live value but the set_bootenv function should postpone the update to the end of the update process. Best regards Stefan Herbrechtsmeier Software Developer Embedded Systems Weidmüller – Your partner in Industrial Connectivity We look forward to sharing ideas with you – Let’s connect. Weidmueller Interface GmbH & Co. KG Klingenbergstraße 16, 32758 Detmold, Germany Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
Hi Stefan, On 17/01/2018 17:45, Stefan.Herbrechtsmeier@weidmueller.com wrote: > Hi Stefano, > >> -----Ursprüngliche Nachricht----- >> Von: Stefano Babic [mailto:sbabic@denx.de] >> Gesendet: Mittwoch, 17. Januar 2018 16:59 >> An: Herbrechtsmeier Dr.-Ing. , Stefan >> <Stefan.Herbrechtsmeier@weidmueller.com>; sbabic@denx.de; >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com >> Betreff: Re: AW: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get >> and set functions to Lua scripts >> >> On 17/01/2018 16:38, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>> Hi Stefano, >>> >>>> -----Ursprüngliche Nachricht----- >>>> Von: Stefano Babic [mailto:sbabic@denx.de] >>>> Gesendet: Mittwoch, 17. Januar 2018 15:48 >>>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com >>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan >>>> <Stefan.Herbrechtsmeier@weidmueller.com> >>>> Betreff: Re: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get and >>>> set functions to Lua scripts >>>> >>>> On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: >>>>> From: Stefan Herbrechtsmeier >>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>> >>>>> Signed-off-by: Stefan Herbrechtsmeier >>>>> <stefan.herbrechtsmeier@weidmueller.com> >>>>> --- >>>>> >>>>> corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 32 insertions(+) >>>>> >>>>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index >>>>> 25d84e6..f370474 100644 >>>>> --- a/corelib/lua_interface.c >>>>> +++ b/corelib/lua_interface.c >>>>> @@ -16,6 +16,7 @@ >>>>> #include "lua_util.h" >>>>> #include "util.h" >>>>> #include "handler.h" >>>>> +#include "bootloader.h" >>>>> >>>>> #define LUA_TYPE_PEMBSCR 1 >>>>> #define LUA_TYPE_HANDLER 2 >>>>> @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { >>>>> return 0; >>>>> } >>>>> >>>>> +static int l_get_bootenv(lua_State *L) { >>>>> + const char *name = luaL_checkstring(L, 1); >>>>> + char *value = NULL; >>>>> + >>>>> + if (strlen(name)) >>>>> + value = bootloader_env_get(name); >>>>> + lua_pop(L, 1); >>>>> + >>>>> + lua_pushstring(L, value); >>>>> + free(value); >>>>> + >>>>> + return 1; >>>>> +} >>>>> + >>>>> +static int l_set_bootenv(lua_State *L) { >>>>> + const char *name = luaL_checkstring(L, 1); >>>>> + const char *value = luaL_checkstring(L, 2); >>>>> + >>>>> + if (strlen(name)) { >>>>> + if (strlen(value)) >>>>> + bootloader_env_set(name, value); >>>>> + else >>>>> + bootloader_env_unset(name); >>>>> + } >>>>> + lua_pop(L, 2); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> #ifdef CONFIG_HANDLER_IN_LUA >>>>> static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ static >>>>> const luaL_Reg l_swupdate[] = { >>>>> { "error", l_error }, >>>>> { "trace", l_trace }, >>>>> { "info", l_info }, >>>>> + { "get_bootenv", l_get_bootenv }, >>>>> + { "set_bootenv", l_set_bootenv }, >>>>> { NULL, NULL } >>>>> }; >>>>> >>>>> >>>> >>>> A set() could be very dangerous. I can understand that it could be >>>> helpful, but it is very difficult to track when something is going >>>> wronf and could break the system. >>>> >>>> Think about this scenario: a LUA (jandler, script) is accessing to >>>> the environment and changes. Everything fine, after that the next >>>> handler finds an error and aborts the installation. The system >>>> remains with the environment updated by the LUA actor, not with the >>>> original - system does not maybe boot anymore. >>>> >>>> This is the reason why u-boot is set at the end of the update: all >>>> handlers ran, and we are sure that upgrading the environment is the >>>> last step. Even if it fails, we have still the old environment. >>>> >>>> But if a LUA script can do this, everything happens. >>>> >>>> I agree that someone (I read from ML) has the good idea to use >>>> fw_setnev inside shell scripts - another way to try to brick the device. >>>> >>>> IMHO this looks dangerous.... >>> >>> I understand your concerns but the "-e" parameter has also the potential >> to break your system if you don't restart the system between two updates. >> >> Not exactly - you are talking about a misuse. SWUpdate is more a framework, >> and if the update concept is wrong, there is nothing that can be done. You >> example applies just to double-copy, and the designer should take care that >> a successful update requires a restart. If the design is correct, there is no risk. >> >>> Anyway let's keep the risk low if other solution exists. >>> >>> Alternative I can add support for a Lua hook script to the bootloader >> environment entries to change the value at parse time and execute the >> changes at the end of the update. >> >> Maybe we are thinking the same. I thought to have a "set_bootenv" that >> handles the bootloader list. The update will be done at the end as usual. Is >> this what you mind ? > > I mean a hook to an embedded-script function in the sw-description: > bootenv: ( > { > name = "vram"; > value = "4M"; > hook = "my_lua_script"; > } > ); This is just a use case, that is when the variables are manipulated at parse time. We can have LUA scripts, and LUA handlers, too. > > Do you mean a set_bootenv function which only updates the bootloader environment dictionary? Yes, this looks to me more generic. As far as I see, you do not need this now and it could be postpone until a use case appears. > How will you pass the bootloader dictionary to this function? I have not thought to pass the whole dictionary, but I confess I have not deeply thought how to implement it. > >>> But therefore I need to extend and temporally misuse the img_type to >> pass the name and value to a Lua handler. >>> >> >> mmmhh...get_bootenv in LUA returns the environment, and >> set_bootenv(var, >> value) will change just a variable in the list. Not sure whhat you plan to do. > > I need a function to read the current bootloader value. Therefore the get_bootenv should return the live value but the set_bootenv function should postpone the update to the end of the update process. > Ok Best regards, Stefano
Hi Stefano, > -----Ursprüngliche Nachricht----- > Von: Stefano Babic [mailto:sbabic@denx.de] > Gesendet: Mittwoch, 17. Januar 2018 18:09 > An: Herbrechtsmeier Dr.-Ing. , Stefan > <Stefan.Herbrechtsmeier@weidmueller.com>; sbabic@denx.de; > stefan@herbrechtsmeier.net; swupdate@googlegroups.com > Betreff: Re: AW: AW: [swupdate] [PATCH] Lua: Export SWUpdate bootenv > get and set functions to Lua scripts > > On 17/01/2018 17:45, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >> -----Ursprüngliche Nachricht----- > >> Von: Stefano Babic [mailto:sbabic@denx.de] > >> Gesendet: Mittwoch, 17. Januar 2018 16:59 > >> An: Herbrechtsmeier Dr.-Ing. , Stefan > >> <Stefan.Herbrechtsmeier@weidmueller.com>; sbabic@denx.de; > >> stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >> Betreff: Re: AW: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get > >> and set functions to Lua scripts > >> > >> On 17/01/2018 16:38, Stefan.Herbrechtsmeier@weidmueller.com wrote: > >>>> -----Ursprüngliche Nachricht----- > >>>> Von: Stefano Babic [mailto:sbabic@denx.de] > >>>> Gesendet: Mittwoch, 17. Januar 2018 15:48 > >>>> An: stefan@herbrechtsmeier.net; swupdate@googlegroups.com > >>>> Cc: Herbrechtsmeier Dr.-Ing. , Stefan > >>>> <Stefan.Herbrechtsmeier@weidmueller.com> > >>>> Betreff: Re: [swupdate] [PATCH] Lua: Export SWUpdate bootenv get > >>>> and set functions to Lua scripts > >>>> > >>>> On 17/01/2018 15:39, stefan@herbrechtsmeier.net wrote: > >>>>> From: Stefan Herbrechtsmeier > >>>> <stefan.herbrechtsmeier@weidmueller.com> > >>>>> > >>>>> Signed-off-by: Stefan Herbrechtsmeier > >>>>> <stefan.herbrechtsmeier@weidmueller.com> > >>>>> --- > >>>>> > >>>>> corelib/lua_interface.c | 32 ++++++++++++++++++++++++++++++++ > >>>>> 1 file changed, 32 insertions(+) > >>>>> > >>>>> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > >>>>> index > >>>>> 25d84e6..f370474 100644 > >>>>> --- a/corelib/lua_interface.c > >>>>> +++ b/corelib/lua_interface.c > >>>>> @@ -16,6 +16,7 @@ > >>>>> #include "lua_util.h" > >>>>> #include "util.h" > >>>>> #include "handler.h" > >>>>> +#include "bootloader.h" > >>>>> > >>>>> #define LUA_TYPE_PEMBSCR 1 > >>>>> #define LUA_TYPE_HANDLER 2 > >>>>> @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { > >>>>> return 0; > >>>>> } > >>>>> > >>>>> +static int l_get_bootenv(lua_State *L) { > >>>>> + const char *name = luaL_checkstring(L, 1); > >>>>> + char *value = NULL; > >>>>> + > >>>>> + if (strlen(name)) > >>>>> + value = bootloader_env_get(name); > >>>>> + lua_pop(L, 1); > >>>>> + > >>>>> + lua_pushstring(L, value); > >>>>> + free(value); > >>>>> + > >>>>> + return 1; > >>>>> +} > >>>>> + > >>>>> +static int l_set_bootenv(lua_State *L) { > >>>>> + const char *name = luaL_checkstring(L, 1); > >>>>> + const char *value = luaL_checkstring(L, 2); > >>>>> + > >>>>> + if (strlen(name)) { > >>>>> + if (strlen(value)) > >>>>> + bootloader_env_set(name, value); > >>>>> + else > >>>>> + bootloader_env_unset(name); > >>>>> + } > >>>>> + lua_pop(L, 2); > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> #ifdef CONFIG_HANDLER_IN_LUA > >>>>> static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ > >>>>> static const luaL_Reg l_swupdate[] = { > >>>>> { "error", l_error }, > >>>>> { "trace", l_trace }, > >>>>> { "info", l_info }, > >>>>> + { "get_bootenv", l_get_bootenv }, > >>>>> + { "set_bootenv", l_set_bootenv }, > >>>>> { NULL, NULL } > >>>>> }; [snip] > >>> Alternative I can add support for a Lua hook script to the > >>> bootloader > >> environment entries to change the value at parse time and execute the > >> changes at the end of the update. > >> > >> Maybe we are thinking the same. I thought to have a "set_bootenv" > >> that handles the bootloader list. The update will be done at the end > >> as usual. Is this what you mind ? > > > > I mean a hook to an embedded-script function in the sw-description: > > bootenv: ( > > { > > name = "vram"; > > value = "4M"; > > hook = "my_lua_script"; > > } > > ); > > This is just a use case, that is when the variables are manipulated at parse > time. We can have LUA scripts, and LUA handlers, too. > > > > > Do you mean a set_bootenv function which only updates the bootloader > environment dictionary? > > Yes, this looks to me more generic. As far as I see, you do not need this now > and it could be postpone until a use case appears. Okay > > > How will you pass the bootloader dictionary to this function? > > I have not thought to pass the whole dictionary, but I confess I have not > deeply thought how to implement it. We could add a pointer of the bootloader environment dictionary to the Lua registry and use the l_set_bootenv function to add a key and value to this dictionary. Therefore we need to pass the dictionary or swupdate_cfg to lua_parser_fn. > >>> But therefore I need to extend and temporally misuse the img_type to > >> pass the name and value to a Lua handler. > >>> > >> > >> mmmhh...get_bootenv in LUA returns the environment, and > >> set_bootenv(var, > >> value) will change just a variable in the list. Not sure whhat you plan to do. > > > > I need a function to read the current bootloader value. Therefore the > get_bootenv should return the live value but the set_bootenv function > should postpone the update to the end of the update process. > > > > Ok What are your naming preferences? swupdate.get_bootenv("var") or bootloader.getenv("var")? Best regards Stefan Herbrechtsmeier Software Developer Embedded Systems Weidmüller – Your partner in Industrial Connectivity We look forward to sharing ideas with you – Let’s connect. Weidmueller Interface GmbH & Co. KG Klingenbergstraße 16, 32758 Detmold, Germany Email: Stefan.Herbrechtsmeier@weidmueller.com – Web: www.weidmueller.com
Hi Stefan, On 18/01/2018 08:56, Stefan.Herbrechtsmeier@weidmueller.com wrote: >>> How will you pass the bootloader dictionary to this function? >> >> I have not thought to pass the whole dictionary, but I confess I have not >> deeply thought how to implement it. > > We could add a pointer of the bootloader environment dictionary to the Lua registry and use the l_set_bootenv function to add a key and value to this dictionary. Therefore we need to pass the dictionary or swupdate_cfg to lua_parser_fn. Yes, something like that, but we have to be sure about the context. Until everything is running in the installer (=the main process), this is fine. Anyway, LUA runs in the installer context. > >>>>> But therefore I need to extend and temporally misuse the img_type to >>>> pass the name and value to a Lua handler. >>>>> >>>> >>>> mmmhh...get_bootenv in LUA returns the environment, and >>>> set_bootenv(var, >>>> value) will change just a variable in the list. Not sure whhat you plan to do. >>> >>> I need a function to read the current bootloader value. Therefore the >> get_bootenv should return the live value but the set_bootenv function >> should postpone the update to the end of the update process. >>> >> >> Ok > > What are your naming preferences? > swupdate.get_bootenv("var") or bootloader.getenv("var")? We have a require("swupdate") and functions from LUA are called with the "swupdate" prefix. I prefer the first one to have the names consistent. Best regards, Stefano
diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index 25d84e6..f370474 100644 --- a/corelib/lua_interface.c +++ b/corelib/lua_interface.c @@ -16,6 +16,7 @@ #include "lua_util.h" #include "util.h" #include "handler.h" +#include "bootloader.h" #define LUA_TYPE_PEMBSCR 1 #define LUA_TYPE_HANDLER 2 @@ -648,6 +649,35 @@ static int l_info(lua_State *L) { return 0; } +static int l_get_bootenv(lua_State *L) { + const char *name = luaL_checkstring(L, 1); + char *value = NULL; + + if (strlen(name)) + value = bootloader_env_get(name); + lua_pop(L, 1); + + lua_pushstring(L, value); + free(value); + + return 1; +} + +static int l_set_bootenv(lua_State *L) { + const char *name = luaL_checkstring(L, 1); + const char *value = luaL_checkstring(L, 2); + + if (strlen(name)) { + if (strlen(value)) + bootloader_env_set(name, value); + else + bootloader_env_unset(name); + } + lua_pop(L, 2); + + return 0; +} + #ifdef CONFIG_HANDLER_IN_LUA static int l_get_tmpdir(lua_State *L) { @@ -664,6 +694,8 @@ static const luaL_Reg l_swupdate[] = { { "error", l_error }, { "trace", l_trace }, { "info", l_info }, + { "get_bootenv", l_get_bootenv }, + { "set_bootenv", l_set_bootenv }, { NULL, NULL } };