| Message ID | YfAfO1Rm1uKIiCr0@decadent.org.uk |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] handlers: Change parameters to allow passing both data and scriptfn | expand |
Hi Ben, On 25.01.22 17:03, Ben Hutchings wrote: > Handlers are normally called with their opaque data pointer as the > second argument. However, for script handlers the second argument is > a pointer to the script_fn (PREINSTALL or POSTINSTALL) indicating > which phase the handler is being called for. > > The Lua handler wrapper code stores a reference to the function as the > handler data, so a handler written in Lua cannot be a script handler. > > Change the handler function type to have separate data and scriptfn > parameters, and update all callers and implementations accordingly. > Before going deep into the review, I just ask why you think its is not working. Lua has much more possibility as shell scripts to get parameters / data, and they are already used in SWUpdate. To be honbest, I think about to change an API (the one for the handlers) if it is not strictly required, and I do not understand why a handler should become script_fn scriptfn) when it is a handler and not a script (and the parameter makes no sense). You are mixing here (shell) script handler with Lua handlers, but they are thought to different use cases. A Lua script (not handler) already receives the same parameters as a shell script, see handlers/lua_scripthandler.c: ret = run_lua_script(filename, fnname, img->type_data); And a Lua handler receives the whole image structure created by parser, that means a custoim parameter like: data = "my awsome parameter"; can be retrieved in the Lua code with parameter = image.data So it is not clear which use case is missing and why the fingertip for the handler must be changed. Best regards, Stefano Babic > Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> > --- > core/installer.c | 4 ++-- > corelib/lua_interface.c | 6 ++++-- > doc/source/handlers.rst | 3 ++- > handlers/archive_handler.c | 3 ++- > handlers/boot_handler.c | 3 ++- > handlers/delta_handler.c | 3 ++- > handlers/diskformat_handler.c | 3 ++- > handlers/diskpart_handler.c | 3 ++- > handlers/dummy_handler.c | 3 ++- > handlers/flash_hamming1_handler.c | 3 ++- > handlers/flash_handler.c | 3 ++- > handlers/lua_scripthandler.c | 10 +++------- > handlers/raw_handler.c | 9 ++++++--- > handlers/rdiff_handler.c | 3 ++- > handlers/readback_handler.c | 8 +++----- > handlers/remote_handler.c | 3 ++- > handlers/shell_scripthandler.c | 32 +++++++++---------------------- > handlers/ssbl_handler.c | 10 +++------- > handlers/swuforward_handler.c | 3 ++- > handlers/ubivol_handler.c | 16 +++++++--------- > handlers/ucfw_handler.c | 3 ++- > handlers/uniqueuuid_handler.c | 3 ++- > include/handler.h | 2 +- > 23 files changed, 66 insertions(+), 73 deletions(-) > > diff --git a/core/installer.c b/core/installer.c > index 5e4cbe5..eb07f41 100644 > --- a/core/installer.c > +++ b/core/installer.c > @@ -200,7 +200,7 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) > continue; > hnd = find_handler(img); > if (hnd) { > - ret = hnd->installer(img, &type); > + ret = hnd->installer(img, hnd->data, type); > if (ret) > return ret; > } > @@ -231,7 +231,7 @@ int install_single_image(struct img_type *img, bool dry_run) > swupdate_progress_inc_step(img->fname, hnd->desc); > > /* TODO : check callback to push results / progress */ > - ret = hnd->installer(img, hnd->data); > + ret = hnd->installer(img, hnd->data, NONE); > if (ret != 0) { > TRACE("Installer for %s not successful !", > hnd->desc); > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c > index 843c0a9..8045d73 100644 > --- a/corelib/lua_interface.c > +++ b/corelib/lua_interface.c > @@ -1070,7 +1070,9 @@ static lua_State *gL = NULL; > * @param data [in] pointer to the index in the Lua registry for the function > * @return This function returns 0 if successful and -1 if unsuccessful. > */ > -static int l_handler_wrapper(struct img_type *img, void *data) { > +static int l_handler_wrapper(struct img_type *img, void *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > +{ > int res = 0; > lua_Number result; > int l_func_ref; > @@ -1175,7 +1177,7 @@ static int l_call_handler(lua_State *L) > ret = 1; > goto call_handler_exit; > } > - if ((hnd->installer(&img, hnd->data)) != 0) { > + if ((hnd->installer(&img, hnd->data, NONE)) != 0) { > if (asprintf(&msg, "Executing handler %s failed!", hnd->desc) == -1) { > msg = NULL; > } > diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst > index 333c912..6d5be62 100644 > --- a/doc/source/handlers.rst > +++ b/doc/source/handlers.rst > @@ -56,7 +56,8 @@ The prototype for the callback is: > :: > > int my_handler(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > > > The most important parameter is the pointer to a struct img_type. It describes > diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c > index ef8989f..f794048 100644 > --- a/handlers/archive_handler.c > +++ b/handlers/archive_handler.c > @@ -210,7 +210,8 @@ out: > } > > static int install_archive_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > char path[255]; > int fdout = -1; > diff --git a/handlers/boot_handler.c b/handlers/boot_handler.c > index 73c9116..f16650e 100644 > --- a/handlers/boot_handler.c > +++ b/handlers/boot_handler.c > @@ -25,7 +25,8 @@ static void uboot_handler(void); > static void boot_handler(void); > > static int install_boot_environment(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret; > int fdout; > diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c > index 9a91352..98ec860 100644 > --- a/handlers/delta_handler.c > +++ b/handlers/delta_handler.c > @@ -869,7 +869,8 @@ static bool copy_existing_chunks(zckChunk **dstChunk, struct hnd_priv *priv) > * Handler entry point > */ > static int install_delta(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > struct hnd_priv *priv; > int ret = -1; > diff --git a/handlers/diskformat_handler.c b/handlers/diskformat_handler.c > index bf236b2..96f5a5b 100644 > --- a/handlers/diskformat_handler.c > +++ b/handlers/diskformat_handler.c > @@ -15,7 +15,8 @@ > void diskformat_handler(void); > > static int diskformat(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret = 0; > > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c > index d96d4e2..6359d8b 100644 > --- a/handlers/diskpart_handler.c > +++ b/handlers/diskpart_handler.c > @@ -774,7 +774,8 @@ static void diskpart_unref_context(struct fdisk_context *cxt) > } > > static int diskpart(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > char *lbtype = diskpart_get_lbtype(img); > struct dict_list *parts; > diff --git a/handlers/dummy_handler.c b/handlers/dummy_handler.c > index c17b55d..bdbc711 100644 > --- a/handlers/dummy_handler.c > +++ b/handlers/dummy_handler.c > @@ -19,7 +19,8 @@ > void dummy_handler(void); > > static int install_nothing(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret; > int fdout; > diff --git a/handlers/flash_hamming1_handler.c b/handlers/flash_hamming1_handler.c > index 7cdb40a..7f092a5 100644 > --- a/handlers/flash_hamming1_handler.c > +++ b/handlers/flash_hamming1_handler.c > @@ -277,7 +277,8 @@ out: > } > > static int install_flash_hamming_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int mtdnum; > > diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c > index 5c11ff6..69da777 100644 > --- a/handlers/flash_handler.c > +++ b/handlers/flash_handler.c > @@ -335,7 +335,8 @@ static int flash_write_image(int mtdnum, struct img_type *img) > } > > static int install_flash_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int mtdnum; > > diff --git a/handlers/lua_scripthandler.c b/handlers/lua_scripthandler.c > index cc47f53..f096fda 100644 > --- a/handlers/lua_scripthandler.c > +++ b/handlers/lua_scripthandler.c > @@ -25,20 +25,16 @@ > > static void lua_handler(void); > > -static int start_lua_script(struct img_type *img, void *data) > +static int start_lua_script(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > int ret; > const char *fnname; > - script_fn scriptfn; > > const char* tmp = get_tmpdirscripts(); > char filename[MAX_IMAGE_FNAME + strlen(tmp) + 2 + strlen(img->type_data)]; > > - if (!data) > - return -1; > - > - scriptfn = *(script_fn *)data; > - > switch (scriptfn) { > case PREINSTALL: > fnname="preinst"; > diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c > index 619a2f4..411cdb2 100644 > --- a/handlers/raw_handler.c > +++ b/handlers/raw_handler.c > @@ -119,7 +119,8 @@ blkprotect_out: > } > > static int install_raw_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret; > int fdout; > @@ -150,7 +151,8 @@ static int install_raw_image(struct img_type *img, > } > > static int copy_raw_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret; > int fdout, fdin; > @@ -203,7 +205,8 @@ static int copy_raw_image(struct img_type *img, > } > > static int install_raw_file(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > char path[255]; > int fdout; > diff --git a/handlers/rdiff_handler.c b/handlers/rdiff_handler.c > index 0b9e146..130ac88 100644 > --- a/handlers/rdiff_handler.c > +++ b/handlers/rdiff_handler.c > @@ -219,7 +219,8 @@ static int apply_rdiff_chunk_cb(void *out, const void *buf, unsigned int len) > } > > static int apply_rdiff_patch(struct img_type *img, > - void __attribute__((__unused__)) * data) > + void __attribute__((__unused__)) * data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > int ret = 0; > > diff --git a/handlers/readback_handler.c b/handlers/readback_handler.c > index 98ffd3d..74dea09 100644 > --- a/handlers/readback_handler.c > +++ b/handlers/readback_handler.c > @@ -26,12 +26,10 @@ > void readback_handler(void); > static int readback_postinst(struct img_type *img); > > -static int readback(struct img_type *img, void *data) > +static int readback(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > - if (!data) > - return -1; > - > - script_fn scriptfn = *(script_fn *)data; > switch (scriptfn) { > case POSTINSTALL: > return readback_postinst(img); > diff --git a/handlers/remote_handler.c b/handlers/remote_handler.c > index 8558d88..8c04773 100644 > --- a/handlers/remote_handler.c > +++ b/handlers/remote_handler.c > @@ -149,7 +149,8 @@ static int forward_data(void *request, const void *buf, unsigned int len) > } > > static int install_remote_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > void *context = zmq_ctx_new(); > void *request = zmq_socket (context, ZMQ_REQ); > diff --git a/handlers/shell_scripthandler.c b/handlers/shell_scripthandler.c > index 9591350..9518216 100644 > --- a/handlers/shell_scripthandler.c > +++ b/handlers/shell_scripthandler.c > @@ -47,15 +47,11 @@ static int execute_shell_script(struct img_type *img, const char *fnname) > return ret; > } > > -static int start_shell_script(struct img_type *img, void *data) > +static int start_shell_script(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > const char *fnname; > - script_fn scriptfn; > - > - if (!data) > - return -EINVAL; > - > - scriptfn = *(script_fn *)data; > > switch (scriptfn) { > case PREINSTALL: > @@ -72,15 +68,10 @@ static int start_shell_script(struct img_type *img, void *data) > return execute_shell_script(img, fnname); > } > > -static int start_preinstall_script(struct img_type *img, void *data) > +static int start_preinstall_script(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > - script_fn scriptfn; > - > - if (!data) > - return -EINVAL; > - > - scriptfn = *(script_fn *)data; > - > /* > * Call only in case of preinstall > */ > @@ -90,15 +81,10 @@ static int start_preinstall_script(struct img_type *img, void *data) > return execute_shell_script(img, ""); > } > > -static int start_postinstall_script(struct img_type *img, void *data) > +static int start_postinstall_script(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > - script_fn scriptfn; > - > - if (!data) > - return -EINVAL; > - > - scriptfn = *(script_fn *)data; > - > /* > * Call only in case of postinstall > */ > diff --git a/handlers/ssbl_handler.c b/handlers/ssbl_handler.c > index 478319f..22b96d1 100644 > --- a/handlers/ssbl_handler.c > +++ b/handlers/ssbl_handler.c > @@ -164,9 +164,10 @@ static int inline get_active_ssbl(struct ssbl_priv *padmins) { > return get_inactive_ssbl(padmins) == 1 ? 0 : 1; > } > > -static int ssbl_swap(struct img_type *img, void *data) > +static int ssbl_swap(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > - script_fn scriptfn; > struct ssbl_priv admins[2]; > struct ssbl_priv *pssbl; > struct proplist *entry; > @@ -175,11 +176,6 @@ static int ssbl_swap(struct img_type *img, void *data) > struct flash_description *flash = get_flash_info(); > char mtd_device[80]; > > - if (!data) > - return -EINVAL; > - > - scriptfn = *(script_fn *)data; > - > /* > * Call only in case of postinstall > */ > diff --git a/handlers/swuforward_handler.c b/handlers/swuforward_handler.c > index 3135afc..fdf765b 100644 > --- a/handlers/swuforward_handler.c > +++ b/handlers/swuforward_handler.c > @@ -284,7 +284,8 @@ static int initialize_backchannel(struct hnd_priv *priv) > } > > static int install_remote_swu(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > struct hnd_priv priv; > struct curlconn *conn, *tmp; > diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c > index 8bf4f30..1960022 100644 > --- a/handlers/ubivol_handler.c > +++ b/handlers/ubivol_handler.c > @@ -388,7 +388,8 @@ static int wait_volume(struct img_type *img) > } > > static int install_ubivol_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > struct flash_description *flash = get_flash_info(); > struct ubi_part *ubivol; > @@ -429,7 +430,8 @@ static int install_ubivol_image(struct img_type *img, > } > > static int adjust_volume(struct img_type *cfg, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > return resize_volume(cfg, cfg->partsize); > } > @@ -450,9 +452,10 @@ static int ubi_volume_get_info(char *name, int *dev_num, int *vol_id) > return 0; > } > > -static int swap_volume(struct img_type *img, void *data) > +static int swap_volume(struct img_type *img, > + void __attribute__ ((__unused__)) *data, > + script_fn scriptfn) > { > - script_fn scriptfn; > struct flash_description *flash = get_flash_info(); > libubi_t libubi = flash->libubi; > int num, count = 0; > @@ -465,11 +468,6 @@ static int swap_volume(struct img_type *img, void *data) > struct ubi_rnvol_req rnvol; > int ret = -1; > > - if (!data) > - return -EINVAL; > - > - scriptfn = *(script_fn *)data; > - > /* > * Call only in case of postinstall > */ > diff --git a/handlers/ucfw_handler.c b/handlers/ucfw_handler.c > index c4f3199..864ba8d 100644 > --- a/handlers/ucfw_handler.c > +++ b/handlers/ucfw_handler.c > @@ -498,7 +498,8 @@ static int get_gpio_from_property(struct dict_list *prop, struct mode_setup *gpi > } > > static int install_uc_firmware_image(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > struct handler_priv hnd_data; > struct dict_list *properties; > diff --git a/handlers/uniqueuuid_handler.c b/handlers/uniqueuuid_handler.c > index 3ffb8f1..a9aa13a 100644 > --- a/handlers/uniqueuuid_handler.c > +++ b/handlers/uniqueuuid_handler.c > @@ -28,7 +28,8 @@ > void uniqueuuid_handler(void); > > static int uniqueuuid(struct img_type *img, > - void __attribute__ ((__unused__)) *data) > + void __attribute__ ((__unused__)) *data, > + script_fn __attribute__ ((__unused__)) scriptfn) > { > struct dict_list *uuids; > struct dict_list_elem *uuid; > diff --git a/include/handler.h b/include/handler.h > index a060623..0807021 100644 > --- a/include/handler.h > +++ b/include/handler.h > @@ -33,7 +33,7 @@ typedef enum { > BOOTLOADER_HANDLER | PARTITION_HANDLER | \ > NO_DATA_HANDLER) > > -typedef int (*handler)(struct img_type *img, void *data); > +typedef int (*handler)(struct img_type *img, void *data, script_fn scriptfn); > struct installer_handler{ > char desc[64]; > handler installer;
Stefano Babic wrote: [...] > Before going deep into the review, I just ask why you think its is not > working. [...] Let me explain what I'm actually doing. I'm working with devices that have fairly limited flash and RAM storage, but they do support dual bank. Update images have to be installed directly due to the lack of temporary storage. The boot loader must be reconfigured before this, to prevent booting from an incomplete image (or one that has been maliciously modified, since its checksum will only be verified after it is written). The images themselves are written with the standard ubivol handler. The boot loader is a version of U-Boot whose environment block is not accessible through libubootenv, and it is configured with vendor- specific metadata accessed through a vendor-specific library. I do not expect to be able to add support for this to upstream SWUpdate, and we don't want to have to maintain internal patches to add it. I originally implemented support for this boot loader through a script handler written in C, requiring no actual script in the image but just properties. I then rewrote it in Lua, putting the necessary bindings to the vendor library in a shared library that Lua can load. That required the changes in this series, which I believed to be suitable for upstream while direct support for this boot loader would not be. To counter the obvious objections: - We can't simply use a custom image handler for this, because there are two separate images to be installed between the preinstall and postinstall actions. - An embedded Lua script would still be dependent on the bindings library being installed. So newer versions of the embedded script would always need to be compatible with older versions of the bindings library. Besides which, it is already possible to register Lua code as a script handler, but this will result in a crash. That is surely a bug. Ben.
Hi Ben, On 26.01.22 17:28, Ben HUTCHINGS (EXT) wrote: > Stefano Babic wrote: > [...] >> Before going deep into the review, I just ask why you think its is not >> working. > [...] > > Let me explain what I'm actually doing. > > I'm working with devices that have fairly limited flash and RAM storage, > but they do support dual bank. Update images have to be installed > directly due to the lack of temporary storage. This is one of the reasons why streaming was implemented ;-). > The boot loader must be > reconfigured before this, to prevent booting from an incomplete image > (or one that has been maliciously modified, since its checksum will only > be verified after it is written). Ok > The images themselves are written with > the standard ubivol handler. Ok > > The boot loader is a version of U-Boot whose environment block is not > accessible through libubootenv, and it is configured with vendor- > specific metadata accessed through a vendor-specific library. ...and I hope this library is provided as open source, else it does not complain with U-Boot's GPLv2. And I have not seen such vendor specific changes on U-Boot's ML (wher eI am one of maintainer..). > I do not > expect to be able to add support for this to upstream SWUpdate, and > we don't want to have to maintain internal patches to add it. Ok > > I originally implemented support for this boot loader through a > script handler written in C, requiring no actual script in the image > but just properties. I then rewrote it in Lua, putting the necessary > bindings to the vendor library in a shared library that Lua can > load. Ok, so fas so good, I got it. > That required the changes in this series, which I believed to > be suitable for upstream while direct support for this boot loader > would not be. Changes in the API with not required parameter for all handlers is something I bother. > > To counter the obvious objections: > - We can't simply use a custom image handler for this, because there > are two separate images to be installed between the preinstall and > postinstall actions. > - An embedded Lua script would still be dependent on the bindings > library being installed. You mean this special vendor library to access the environemnt. Anyway, this is not very different, because SWUpdate has also a strong dependency from libubootenv, so the only way is to track and maintain the API between Lua and the library consistent. As I understand, you can have a middleware library (this is in your hands) to interface Lua with the vendor library. > So newer versions of the embedded script > would always need to be compatible with older versions of the > bindings library. I suggest you add a middleware library that interface with the vendor's and add Lua bindings to overcome this. Anyway, what are the specific parts done by this vendor library (and if I can ask, who is this vendor ?) that are not covered by the open source counterparts ? I am also curious because there is no need for all of this with standard U-Boot (and libubootenv), and an incomplete image is never booted, of course. > > Besides which, it is already possible to register Lua code as a script > handler, but this will result in a crash. That is surely a bug. Surely a bug, but what you mean with "register Lua code as a script handler" ? There are Lua handlers (that is handlers written in Lua, they manage to install an image with a custom type), and scripts written in Lua. Only handlers can be registered tp SWUpdate, Lua scripts don't. Best regards, Stefano Babic
Stefano Babic wrote: > Hi Ben, > > On 26.01.22 17:28, Ben HUTCHINGS (EXT) wrote: [...] > > The boot loader is a version of U-Boot whose environment block is not > > accessible through libubootenv, and it is configured with vendor- > > specific metadata accessed through a vendor-specific library. > > ...and I hope this library is provided as open source, else it does not > complain with U-Boot's GPLv2. And I have not seen such vendor specific > changes on U-Boot's ML (wher eI am one of maintainer..). Two different programs accessing the same data structures don't have to have the same license. In any case, the source to the vendored U-Boot and the other library are available to SoftAtHome under open source licenses and are (so far as I know) being provided to downstream recipients. [...] > > That required the changes in this series, which I believed to > > be suitable for upstream while direct support for this boot loader > > would not be. > > Changes in the API with not required parameter for all handlers is > something I bother. The change was very simple to do for all the included handlers. > > > > To counter the obvious objections: > > - We can't simply use a custom image handler for this, because there > > are two separate images to be installed between the preinstall and > > postinstall actions. > > - An embedded Lua script would still be dependent on the bindings > > library being installed. > > You mean this special vendor library to access the environemnt. Anyway, > this is not very different, because SWUpdate has also a strong > dependency from libubootenv, so the only way is to track and maintain > the API between Lua and the library consistent. As I understand, you can > have a middleware library (this is in your hands) to interface Lua with > the vendor library. Yes, I already wrote that library. > > So newer versions of the embedded script > > would always need to be compatible with older versions of the > > bindings library. > > I suggest you add a middleware library that interface with the vendor's > and add Lua bindings to overcome this. > > Anyway, what are the specific > parts done by this vendor library (and if I can ask, who is this vendor > ?) that are not covered by the open source counterparts ? I don't know if I can tell you the vendor. But the boot metadata is: - default boot partition - one-time boot partition (this is a register, not in flash) - for each partition: - valid flag - sequence number > I am also curious because there is no need for all of this with standard > U-Boot (and libubootenv), and an incomplete image is never booted, of > course. I understand that there are standard ways to do this with U-Boot. But vendors invent their own ways to do things, and we have to work with that. > > > > Besides which, it is already possible to register Lua code as a script > > handler, but this will result in a crash. That is surely a bug. > > Surely a bug, but what you mean with "register Lua code as a script > handler" ? There are Lua handlers (that is handlers written in Lua, they > manage to install an image with a custom type), and scripts written in > Lua. Only handlers can be registered tp SWUpdate, Lua scripts don't. I mean that if you put this in swupdate_handlers.lua: local swupdate = require("swupdate") function foo(image) return 0 end swupdate.register_handler("foo", foo, swupdate.HANDLER_MASK.SCRIPT_HANDLER) then try to install an update with a script referencing the foo handler, SWUpdate will crash (I think; I didn't actually try it). I'm not really interested in writing a real script interpreter in Lua, and I don't know if that's practical. But script handlers are useful for any pre- or post-install actions that are not tied to a single image or file, and I think it should be possible to implement them in Lua. Ben.
Hi Ben, On 02.02.22 18:48, Ben HUTCHINGS (EXT) wrote: > Stefano Babic wrote: >> Hi Ben, >> >> On 26.01.22 17:28, Ben HUTCHINGS (EXT) wrote: > [...] >>> The boot loader is a version of U-Boot whose environment block is not >>> accessible through libubootenv, and it is configured with vendor- >>> specific metadata accessed through a vendor-specific library. >> >> ...and I hope this library is provided as open source, else it does not >> complain with U-Boot's GPLv2. And I have not seen such vendor specific >> changes on U-Boot's ML (wher eI am one of maintainer..). > > Two different programs accessing the same data structures don't > have to have the same license. Correct - if the vendor library just reuses the structures without including any file from U-Boot, it can be deployed under any license. > In any case, the source to the > vendored U-Boot and the other library are available to SoftAtHome > under open source licenses and are (so far as I know) being > provided to downstream recipients. > > [...] You are on safest side ;-) >>> That required the changes in this series, which I believed to >>> be suitable for upstream while direct support for this boot loader >>> would not be. >> >> Changes in the API with not required parameter for all handlers is >> something I bother. > > The change was very simple to do for all the included handlers. > It is, but...I like to see if this change in API is consistent. There is no use case for all other handlers but your use case. It is at least arguable why an image or file handler should have as interface "script_fn scriptfn, and I do not see any usage for these handlers. >>> >>> To counter the obvious objections: >>> - We can't simply use a custom image handler for this, because there >>> are two separate images to be installed between the preinstall and >>> postinstall actions. >>> - An embedded Lua script would still be dependent on the bindings >>> library being installed. >> >> You mean this special vendor library to access the environemnt. Anyway, >> this is not very different, because SWUpdate has also a strong >> dependency from libubootenv, so the only way is to track and maintain >> the API between Lua and the library consistent. As I understand, you can >> have a middleware library (this is in your hands) to interface Lua with >> the vendor library. > > Yes, I already wrote that library. ok > >>> So newer versions of the embedded script >>> would always need to be compatible with older versions of the >>> bindings library. >> >> I suggest you add a middleware library that interface with the vendor's >> and add Lua bindings to overcome this. >> >> Anyway, what are the specific >> parts done by this vendor library (and if I can ask, who is this vendor >> ?) that are not covered by the open source counterparts ? > > I don't know if I can tell you the vendor. It does not matter (I had already "converted" some vendor libraries from U-Boot to standard, maybe this is one case I had) - I want just to understand why it is different > But the boot metadata is: > > - default boot partition > - one-time boot partition (this is a register, not in flash) > - for each partition: > - valid flag > - sequence number ok > >> I am also curious because there is no need for all of this with standard >> U-Boot (and libubootenv), and an incomplete image is never booted, of >> course. > > I understand that there are standard ways to do this with U-Boot. > But vendors invent their own ways to do things, and we have to > work with that. got it. > >>> >>> Besides which, it is already possible to register Lua code as a script >>> handler, but this will result in a crash. That is surely a bug. >> >> Surely a bug, but what you mean with "register Lua code as a script >> handler" ? There are Lua handlers (that is handlers written in Lua, they >> manage to install an image with a custom type), and scripts written in >> Lua. Only handlers can be registered tp SWUpdate, Lua scripts don't. > > I mean that if you put this in swupdate_handlers.lua: > > local swupdate = require("swupdate") > function foo(image) > return 0 > end > swupdate.register_handler("foo", foo, swupdate.HANDLER_MASK.SCRIPT_HANDLER) > > then try to install an update with a script referencing the foo > handler, SWUpdate will crash (I think; I didn't actually try it). SWupdate does not crash - I tested your example above to be sure. Of course, an update does not work (it fails as soon as preinstall script should run), but SWUpdate terminates with error code because l_handler_wrapper returns with an error code as you have also found. > > I'm not really interested in writing a real script interpreter in > Lua, and I don't know if that's practical. But script handlers > are useful for any pre- or post-install actions that are not tied > to a single image or file, and I think it should be possible to > implement them in Lua. Yes, but we have already pre-/post-install scripts that can be written as shell or lua scripts (not handler), and I still trying to understand which is the added value introducing a Lua handler for scripts compared to a Lua script called by the lua_scripthandler (type=lua). Best regards, Stefano
Stefano Babic wrote: > Hi Ben, > > On 02.02.22 18:48, Ben HUTCHINGS (EXT) wrote: > > Stefano Babic wrote: [...] > >>> That required the changes in this series, which I believed to > >>> be suitable for upstream while direct support for this boot loader > >>> would not be. > >> > >> Changes in the API with not required parameter for all handlers is > >> something I bother. > > > > The change was very simple to do for all the included handlers. > > > > It is, but...I like to see if this change in API is consistent. There is > no use case for all other handlers but your use case. It is at least > arguable why an image or file handler should have as interface > "script_fn scriptfn, and I do not see any usage for these handlers. That is a consequence of using a single function pointer type for all handlers. An alternative would be to use separate types for image and script handlers. Would you prefer that? [...] > > I'm not really interested in writing a real script interpreter in > > Lua, and I don't know if that's practical. But script handlers > > are useful for any pre- or post-install actions that are not tied > > to a single image or file, and I think it should be possible to > > implement them in Lua. > > Yes, but we have already pre-/post-install scripts that can be written > as shell or lua scripts (not handler), and I still trying to understand > which is the added value introducing a Lua handler for scripts compared > to a Lua script called by the lua_scripthandler (type=lua). The value is that a handler can be tested with and then will always be used with specific versions of Lua modules and other components installed in the system image. A script in an upgrade image has to be compatible with a range of possible installed versions, and that is generally harder to get correct and harder to test thoroughly. Ben.
Hi Ben, On 04.02.22 16:52, Ben HUTCHINGS (EXT) wrote: > Stefano Babic wrote: >> Hi Ben, >> >> On 02.02.22 18:48, Ben HUTCHINGS (EXT) wrote: >>> Stefano Babic wrote: > [...] >>>>> That required the changes in this series, which I believed to >>>>> be suitable for upstream while direct support for this boot loader >>>>> would not be. >>>> >>>> Changes in the API with not required parameter for all handlers is >>>> something I bother. >>> >>> The change was very simple to do for all the included handlers. >>> >> >> It is, but...I like to see if this change in API is consistent. There is >> no use case for all other handlers but your use case. It is at least >> arguable why an image or file handler should have as interface >> "script_fn scriptfn, and I do not see any usage for these handlers. > > That is a consequence of using a single function pointer type for > all handlers. But this was a decision in the *design* of the project. As goal, SWUpdate is quite unaware about the delivered artifacts, and this is (quite) done considering all artifacts at the same level, more or less as "images". This is completely true for images and files: it is more a logical split in sw-description, but SWUpdate stores them in the same way inside its own structure. Artifacts are stored as struct img_type, independently if they belong to images / files / scripts. But yes, scripts are stored in a separate list due to the execution path to call them at a specific time of the update (as pre- or postinstall). But it is supported, too, to write an own Lua handler, that registers itself as image or file handler, and instead of installing the artifact it executes the artifact. This an alternative way for hat you want to achieve if I am not wrong. > An alternative would be to use separate types for > image and script handlers. Would you prefer that? mmhhh....no. I would like to not break the abstraction, so that as much as possible there is no distinction between images / files / scripts, they are simply "blobs" that a suitable handler can manage. There are just a few places where is_script is checked, else they are treated in the same way. > > [...] >>> I'm not really interested in writing a real script interpreter in >>> Lua, and I don't know if that's practical. But script handlers >>> are useful for any pre- or post-install actions that are not tied >>> to a single image or file, and I think it should be possible to >>> implement them in Lua. >> >> Yes, but we have already pre-/post-install scripts that can be written >> as shell or lua scripts (not handler), and I still trying to understand >> which is the added value introducing a Lua handler for scripts compared >> to a Lua script called by the lua_scripthandler (type=lua). > > The value is that a handler can be tested with and then will > always be used with specific versions of Lua modules and other > components installed in the system image. A script in an upgrade > image has to be compatible with a range of possible installed > versions, and that is generally harder to get correct and harder > to test thoroughly. I could be wrong and I am maybe biased because I answered to this thread : https://groups.google.com/g/swupdate/c/AJRIeLrE9bM about reallocating handlers at runtime (my patch is buggy), but it is a chance for me to check if what is needed is to load Lua Handlers at runtime (currently not supported), that is a Lua Handler is packed into the SWU and registers itself to be called later when an artifact of that type must be installed. Then it becomes even more self-contained - applying this to your use case (it looks possible to me), it still remains the dependency with the shared libraries (vendor and yours): the Lua handler (delivered) still depends on these shared libraries on the running system, that is the library in old version. One way could be to deliver the shared libraries,too, and update it in a "sandbox like" for the updater, but well...same issue will be for all shared library. I tried to reduce dependencies and to not add libraries just if I need a couple of functions, but of course big libraries (libc / libcurl / libOpenSSL....) are linked dynamically. Best regards, Stefano
diff --git a/core/installer.c b/core/installer.c index 5e4cbe5..eb07f41 100644 --- a/core/installer.c +++ b/core/installer.c @@ -200,7 +200,7 @@ static int run_prepost_scripts(struct imglist *list, script_fn type) continue; hnd = find_handler(img); if (hnd) { - ret = hnd->installer(img, &type); + ret = hnd->installer(img, hnd->data, type); if (ret) return ret; } @@ -231,7 +231,7 @@ int install_single_image(struct img_type *img, bool dry_run) swupdate_progress_inc_step(img->fname, hnd->desc); /* TODO : check callback to push results / progress */ - ret = hnd->installer(img, hnd->data); + ret = hnd->installer(img, hnd->data, NONE); if (ret != 0) { TRACE("Installer for %s not successful !", hnd->desc); diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c index 843c0a9..8045d73 100644 --- a/corelib/lua_interface.c +++ b/corelib/lua_interface.c @@ -1070,7 +1070,9 @@ static lua_State *gL = NULL; * @param data [in] pointer to the index in the Lua registry for the function * @return This function returns 0 if successful and -1 if unsuccessful. */ -static int l_handler_wrapper(struct img_type *img, void *data) { +static int l_handler_wrapper(struct img_type *img, void *data, + script_fn __attribute__ ((__unused__)) scriptfn) +{ int res = 0; lua_Number result; int l_func_ref; @@ -1175,7 +1177,7 @@ static int l_call_handler(lua_State *L) ret = 1; goto call_handler_exit; } - if ((hnd->installer(&img, hnd->data)) != 0) { + if ((hnd->installer(&img, hnd->data, NONE)) != 0) { if (asprintf(&msg, "Executing handler %s failed!", hnd->desc) == -1) { msg = NULL; } diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst index 333c912..6d5be62 100644 --- a/doc/source/handlers.rst +++ b/doc/source/handlers.rst @@ -56,7 +56,8 @@ The prototype for the callback is: :: int my_handler(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) The most important parameter is the pointer to a struct img_type. It describes diff --git a/handlers/archive_handler.c b/handlers/archive_handler.c index ef8989f..f794048 100644 --- a/handlers/archive_handler.c +++ b/handlers/archive_handler.c @@ -210,7 +210,8 @@ out: } static int install_archive_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { char path[255]; int fdout = -1; diff --git a/handlers/boot_handler.c b/handlers/boot_handler.c index 73c9116..f16650e 100644 --- a/handlers/boot_handler.c +++ b/handlers/boot_handler.c @@ -25,7 +25,8 @@ static void uboot_handler(void); static void boot_handler(void); static int install_boot_environment(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret; int fdout; diff --git a/handlers/delta_handler.c b/handlers/delta_handler.c index 9a91352..98ec860 100644 --- a/handlers/delta_handler.c +++ b/handlers/delta_handler.c @@ -869,7 +869,8 @@ static bool copy_existing_chunks(zckChunk **dstChunk, struct hnd_priv *priv) * Handler entry point */ static int install_delta(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { struct hnd_priv *priv; int ret = -1; diff --git a/handlers/diskformat_handler.c b/handlers/diskformat_handler.c index bf236b2..96f5a5b 100644 --- a/handlers/diskformat_handler.c +++ b/handlers/diskformat_handler.c @@ -15,7 +15,8 @@ void diskformat_handler(void); static int diskformat(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret = 0; diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c index d96d4e2..6359d8b 100644 --- a/handlers/diskpart_handler.c +++ b/handlers/diskpart_handler.c @@ -774,7 +774,8 @@ static void diskpart_unref_context(struct fdisk_context *cxt) } static int diskpart(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { char *lbtype = diskpart_get_lbtype(img); struct dict_list *parts; diff --git a/handlers/dummy_handler.c b/handlers/dummy_handler.c index c17b55d..bdbc711 100644 --- a/handlers/dummy_handler.c +++ b/handlers/dummy_handler.c @@ -19,7 +19,8 @@ void dummy_handler(void); static int install_nothing(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret; int fdout; diff --git a/handlers/flash_hamming1_handler.c b/handlers/flash_hamming1_handler.c index 7cdb40a..7f092a5 100644 --- a/handlers/flash_hamming1_handler.c +++ b/handlers/flash_hamming1_handler.c @@ -277,7 +277,8 @@ out: } static int install_flash_hamming_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int mtdnum; diff --git a/handlers/flash_handler.c b/handlers/flash_handler.c index 5c11ff6..69da777 100644 --- a/handlers/flash_handler.c +++ b/handlers/flash_handler.c @@ -335,7 +335,8 @@ static int flash_write_image(int mtdnum, struct img_type *img) } static int install_flash_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int mtdnum; diff --git a/handlers/lua_scripthandler.c b/handlers/lua_scripthandler.c index cc47f53..f096fda 100644 --- a/handlers/lua_scripthandler.c +++ b/handlers/lua_scripthandler.c @@ -25,20 +25,16 @@ static void lua_handler(void); -static int start_lua_script(struct img_type *img, void *data) +static int start_lua_script(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { int ret; const char *fnname; - script_fn scriptfn; const char* tmp = get_tmpdirscripts(); char filename[MAX_IMAGE_FNAME + strlen(tmp) + 2 + strlen(img->type_data)]; - if (!data) - return -1; - - scriptfn = *(script_fn *)data; - switch (scriptfn) { case PREINSTALL: fnname="preinst"; diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c index 619a2f4..411cdb2 100644 --- a/handlers/raw_handler.c +++ b/handlers/raw_handler.c @@ -119,7 +119,8 @@ blkprotect_out: } static int install_raw_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret; int fdout; @@ -150,7 +151,8 @@ static int install_raw_image(struct img_type *img, } static int copy_raw_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret; int fdout, fdin; @@ -203,7 +205,8 @@ static int copy_raw_image(struct img_type *img, } static int install_raw_file(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { char path[255]; int fdout; diff --git a/handlers/rdiff_handler.c b/handlers/rdiff_handler.c index 0b9e146..130ac88 100644 --- a/handlers/rdiff_handler.c +++ b/handlers/rdiff_handler.c @@ -219,7 +219,8 @@ static int apply_rdiff_chunk_cb(void *out, const void *buf, unsigned int len) } static int apply_rdiff_patch(struct img_type *img, - void __attribute__((__unused__)) * data) + void __attribute__((__unused__)) * data, + script_fn __attribute__ ((__unused__)) scriptfn) { int ret = 0; diff --git a/handlers/readback_handler.c b/handlers/readback_handler.c index 98ffd3d..74dea09 100644 --- a/handlers/readback_handler.c +++ b/handlers/readback_handler.c @@ -26,12 +26,10 @@ void readback_handler(void); static int readback_postinst(struct img_type *img); -static int readback(struct img_type *img, void *data) +static int readback(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { - if (!data) - return -1; - - script_fn scriptfn = *(script_fn *)data; switch (scriptfn) { case POSTINSTALL: return readback_postinst(img); diff --git a/handlers/remote_handler.c b/handlers/remote_handler.c index 8558d88..8c04773 100644 --- a/handlers/remote_handler.c +++ b/handlers/remote_handler.c @@ -149,7 +149,8 @@ static int forward_data(void *request, const void *buf, unsigned int len) } static int install_remote_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { void *context = zmq_ctx_new(); void *request = zmq_socket (context, ZMQ_REQ); diff --git a/handlers/shell_scripthandler.c b/handlers/shell_scripthandler.c index 9591350..9518216 100644 --- a/handlers/shell_scripthandler.c +++ b/handlers/shell_scripthandler.c @@ -47,15 +47,11 @@ static int execute_shell_script(struct img_type *img, const char *fnname) return ret; } -static int start_shell_script(struct img_type *img, void *data) +static int start_shell_script(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { const char *fnname; - script_fn scriptfn; - - if (!data) - return -EINVAL; - - scriptfn = *(script_fn *)data; switch (scriptfn) { case PREINSTALL: @@ -72,15 +68,10 @@ static int start_shell_script(struct img_type *img, void *data) return execute_shell_script(img, fnname); } -static int start_preinstall_script(struct img_type *img, void *data) +static int start_preinstall_script(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { - script_fn scriptfn; - - if (!data) - return -EINVAL; - - scriptfn = *(script_fn *)data; - /* * Call only in case of preinstall */ @@ -90,15 +81,10 @@ static int start_preinstall_script(struct img_type *img, void *data) return execute_shell_script(img, ""); } -static int start_postinstall_script(struct img_type *img, void *data) +static int start_postinstall_script(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { - script_fn scriptfn; - - if (!data) - return -EINVAL; - - scriptfn = *(script_fn *)data; - /* * Call only in case of postinstall */ diff --git a/handlers/ssbl_handler.c b/handlers/ssbl_handler.c index 478319f..22b96d1 100644 --- a/handlers/ssbl_handler.c +++ b/handlers/ssbl_handler.c @@ -164,9 +164,10 @@ static int inline get_active_ssbl(struct ssbl_priv *padmins) { return get_inactive_ssbl(padmins) == 1 ? 0 : 1; } -static int ssbl_swap(struct img_type *img, void *data) +static int ssbl_swap(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { - script_fn scriptfn; struct ssbl_priv admins[2]; struct ssbl_priv *pssbl; struct proplist *entry; @@ -175,11 +176,6 @@ static int ssbl_swap(struct img_type *img, void *data) struct flash_description *flash = get_flash_info(); char mtd_device[80]; - if (!data) - return -EINVAL; - - scriptfn = *(script_fn *)data; - /* * Call only in case of postinstall */ diff --git a/handlers/swuforward_handler.c b/handlers/swuforward_handler.c index 3135afc..fdf765b 100644 --- a/handlers/swuforward_handler.c +++ b/handlers/swuforward_handler.c @@ -284,7 +284,8 @@ static int initialize_backchannel(struct hnd_priv *priv) } static int install_remote_swu(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { struct hnd_priv priv; struct curlconn *conn, *tmp; diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c index 8bf4f30..1960022 100644 --- a/handlers/ubivol_handler.c +++ b/handlers/ubivol_handler.c @@ -388,7 +388,8 @@ static int wait_volume(struct img_type *img) } static int install_ubivol_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { struct flash_description *flash = get_flash_info(); struct ubi_part *ubivol; @@ -429,7 +430,8 @@ static int install_ubivol_image(struct img_type *img, } static int adjust_volume(struct img_type *cfg, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { return resize_volume(cfg, cfg->partsize); } @@ -450,9 +452,10 @@ static int ubi_volume_get_info(char *name, int *dev_num, int *vol_id) return 0; } -static int swap_volume(struct img_type *img, void *data) +static int swap_volume(struct img_type *img, + void __attribute__ ((__unused__)) *data, + script_fn scriptfn) { - script_fn scriptfn; struct flash_description *flash = get_flash_info(); libubi_t libubi = flash->libubi; int num, count = 0; @@ -465,11 +468,6 @@ static int swap_volume(struct img_type *img, void *data) struct ubi_rnvol_req rnvol; int ret = -1; - if (!data) - return -EINVAL; - - scriptfn = *(script_fn *)data; - /* * Call only in case of postinstall */ diff --git a/handlers/ucfw_handler.c b/handlers/ucfw_handler.c index c4f3199..864ba8d 100644 --- a/handlers/ucfw_handler.c +++ b/handlers/ucfw_handler.c @@ -498,7 +498,8 @@ static int get_gpio_from_property(struct dict_list *prop, struct mode_setup *gpi } static int install_uc_firmware_image(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { struct handler_priv hnd_data; struct dict_list *properties; diff --git a/handlers/uniqueuuid_handler.c b/handlers/uniqueuuid_handler.c index 3ffb8f1..a9aa13a 100644 --- a/handlers/uniqueuuid_handler.c +++ b/handlers/uniqueuuid_handler.c @@ -28,7 +28,8 @@ void uniqueuuid_handler(void); static int uniqueuuid(struct img_type *img, - void __attribute__ ((__unused__)) *data) + void __attribute__ ((__unused__)) *data, + script_fn __attribute__ ((__unused__)) scriptfn) { struct dict_list *uuids; struct dict_list_elem *uuid; diff --git a/include/handler.h b/include/handler.h index a060623..0807021 100644 --- a/include/handler.h +++ b/include/handler.h @@ -33,7 +33,7 @@ typedef enum { BOOTLOADER_HANDLER | PARTITION_HANDLER | \ NO_DATA_HANDLER) -typedef int (*handler)(struct img_type *img, void *data); +typedef int (*handler)(struct img_type *img, void *data, script_fn scriptfn); struct installer_handler{ char desc[64]; handler installer;
Handlers are normally called with their opaque data pointer as the second argument. However, for script handlers the second argument is a pointer to the script_fn (PREINSTALL or POSTINSTALL) indicating which phase the handler is being called for. The Lua handler wrapper code stores a reference to the function as the handler data, so a handler written in Lua cannot be a script handler. Change the handler function type to have separate data and scriptfn parameters, and update all callers and implementations accordingly. Signed-off-by: Ben Hutchings <ben.hutchings_ext@softathome.com> --- core/installer.c | 4 ++-- corelib/lua_interface.c | 6 ++++-- doc/source/handlers.rst | 3 ++- handlers/archive_handler.c | 3 ++- handlers/boot_handler.c | 3 ++- handlers/delta_handler.c | 3 ++- handlers/diskformat_handler.c | 3 ++- handlers/diskpart_handler.c | 3 ++- handlers/dummy_handler.c | 3 ++- handlers/flash_hamming1_handler.c | 3 ++- handlers/flash_handler.c | 3 ++- handlers/lua_scripthandler.c | 10 +++------- handlers/raw_handler.c | 9 ++++++--- handlers/rdiff_handler.c | 3 ++- handlers/readback_handler.c | 8 +++----- handlers/remote_handler.c | 3 ++- handlers/shell_scripthandler.c | 32 +++++++++---------------------- handlers/ssbl_handler.c | 10 +++------- handlers/swuforward_handler.c | 3 ++- handlers/ubivol_handler.c | 16 +++++++--------- handlers/ucfw_handler.c | 3 ++- handlers/uniqueuuid_handler.c | 3 ++- include/handler.h | 2 +- 23 files changed, 66 insertions(+), 73 deletions(-)