Move artifact version check to inner parsing loop

Hi,
this is the first patch of a series (see below) meant to enhance Hook
handling.

Commit message reads:

    Move artifact version check to inner parsing loop.
>
>     Artifact version, when install-if-differet/install-if-higher is
> specified,
>     is now done before call to parsing hook (if any).
>
>     To do this:
>     - functions is_image_installed() and is_image_higher() moved from
> core/parser.c
>       to parser/parser.c
>     - calls to these functions, together with loops in
> remove_installed_image_list()
>       and remove_higher_image_list() were completely removed from
> core/parser.c
>     - boolean `required` in `struct img_type` was replaced with `enum
> skip_t skip`
>     - field `img_type.skip` is assigned a value detailing reason for skip
> (if any)
>     - field `img_type.skip` is inserted into table passed to Lua hook (if
> any) end
>       retrieved after Lua call so hook function can override setting.
>     - if `img_type.skip` is non-zero the image is considered "skippable"
> and the
>       whole `img_type` structure is removed from handling list.
>
>     Signed-off-by: Mauro Condarelli <mc5686@mclink.it>
>

Next patches will:

   1. make `embedded-script` Lua environment "permanent" (i.e.: not
   constrained to parsing).
   2. add `pre-install-hook` and `post-install-hook` to artifacts.
   3. ensure these hooks are called at the appropriate moment regardless of
   `installed-directly` setting.
   4. add a series of standard hooks (to be defined in `embedded-script`)
   to be called at specific points; e.g.: startup, before-parsing,
   after-parsing, before-installation, after-installation, before-bootvars,
   after-bootvars, on-error.
   5. all  above hooks will be defined in `embedded-script`and they will
   share a single environment, so that variables set in one hook will be
   available to all hooks called after that.

As always comments and criticism are most welcome.

Regards
Mauro

Hi Mauro,

can't you send the patch inline (as you did before), maybe using git 
send-email ? It is difficult to add comments if the patch is attached.

Best regards,
Stefano Babic

On 28.08.20 16:26, Mauro Condarelli wrote:
> Hi,
> this is the first patch of a series (see below) meant to enhance Hook 
> handling.
> 
> Commit message reads:
> 
>          Move artifact version check to inner parsing loop.
> 
>          Artifact version, when install-if-differet/install-if-higher is
>     specified,
>          is now done before call to parsing hook (if any).
> 
>          To do this:
>          - functions is_image_installed() and is_image_higher() moved
>     from core/parser.c
>            to parser/parser.c
>          - calls to these functions, together with loops in
>     remove_installed_image_list()
>            and remove_higher_image_list() were completely removed from
>     core/parser.c
>          - boolean `required` in `struct img_type` was replaced with
>     `enum skip_t skip`
>          - field `img_type.skip` is assigned a value detailing reason
>     for skip (if any)
>          - field `img_type.skip` is inserted into table passed to Lua
>     hook (if any) end
>            retrieved after Lua call so hook function can override setting.
>          - if `img_type.skip` is non-zero the image is considered
>     "skippable" and the
>            whole `img_type` structure is removed from handling list.
> 
>          Signed-off-by: Mauro Condarelli <mc5686@mclink.it
>     <mailto:mc5686@mclink.it>>
> 
> 
> Next patches will:
> 
>  1. make `embedded-script` Lua environment "permanent" (i.e.: not
>     constrained to parsing).
>  2. add `pre-install-hook` and `post-install-hook` to artifacts.
>  3. ensure these hooks are called at the appropriate moment regardless
>     of `installed-directly` setting.
>  4. add a series of standard hooks (to be defined in `embedded-script`)
>     to be called at specific points; e.g.: startup, before-parsing,
>     after-parsing, before-installation, after-installation,
>     before-bootvars, after-bootvars, on-error.
>  5. all  above hooks will be defined in `embedded-script`and they will
>     share a single environment, so that variables set in one hook will
>     be available to all hooks called after that.
> 
> As always comments and criticism are most welcome.
> 
> Regards
> Mauro
> 
> -- 
> You received this message because you are subscribed to the Google 
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send 
> an email to swupdate+unsubscribe@googlegroups.com 
> <mailto:swupdate+unsubscribe@googlegroups.com>.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com 
> <https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com?utm_medium=email&utm_source=footer>.

Here it comes ;)
Sorry for the mishap.
I didn't use `git send-email` becauseI wanted to keep to this thread.
Next version (with more implementation) I will switch to git.

Regards
Mauro

From 8279090b0268d7efac63b5cb9bf8d08ebf68c529 Mon Sep 17 00:00:00 2001
From: Mauro Condarelli <mc5686@mclink.it>
Date: Fri, 28 Aug 2020 15:36:20 +0200
Subject: [PATCH] Move artifact version check to inner parsing loop.

Artifact version, when install-if-differet/install-if-higher is specified,
is now done before call to parsing hook (if any).

To do this:
- functions is_image_installed() and is_image_higher() moved from
core/parser.c
  to parser/parser.c
- calls to these functions, together with loops in
remove_installed_image_list()
  and remove_higher_image_list() were completely removed from core/parser.c
- boolean `required` in `struct img_type` was replaced with `enum skip_t
skip`
- field `img_type.skip` is assigned a value detailing reason for skip (if
any)
- field `img_type.skip` is inserted into table passed to Lua hook (if any)
end
  retrieved after Lua call so hook function can override setting.
- if `img_type.skip` is non-zero the image is considered "skippable" and the
  whole `img_type` structure is removed from handling list.

Signed-off-by: Mauro Condarelli <mc5686@mclink.it>
---
 core/parser.c           | 98 -----------------------------------------
 core/stream_interface.c |  2 +-
 corelib/lua_interface.c |  5 ++-
 include/swupdate.h      |  9 +++-
 parser/parser.c         | 91 +++++++++++++++++++++++++++++++++-----
 5 files changed, 93 insertions(+), 112 deletions(-)

diff --git a/core/parser.c b/core/parser.c
index 7f281cf..bc13d26 100644
--- a/core/parser.c
+++ b/core/parser.c
@@ -130,100 +130,6 @@ static int check_handler_list(struct imglist *list,
  return 0;
 }

-static int is_image_installed(struct swver *sw_ver_list,
- struct img_type *img)
-{
- struct sw_version *swver;
-
- if (!sw_ver_list)
- return false;
-
- if (!strlen(img->id.name) || !strlen(img->id.version) ||
- !img->id.install_if_different)
- return false;
-
- LIST_FOREACH(swver, sw_ver_list, next) {
- /*
- * Check if name and version are identical
- */
- if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) &&
-    !compare_versions(img->id.version, swver->version)) {
- TRACE("%s(%s) already installed, skipping...",
- img->id.name,
- img->id.version);
-
- return true;
- }
- }
-
- return false;
-}
-
-static int is_image_higher(struct swver *sw_ver_list,
- struct img_type *img)
-{
- struct sw_version *swver;
-
- if (!sw_ver_list)
- return false;
-
- if (!strlen(img->id.name) || !strlen(img->id.version) ||
- !img->id.install_if_higher)
- return false;
-
- LIST_FOREACH(swver, sw_ver_list, next) {
- const char* current_version = swver->version;
- const char* proposed_version = img->id.version;
-
- /*
- * Check if name are identical and the new version is lower
- * or equal.
- */
- if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) &&
-    (compare_versions(proposed_version, current_version) < 0)) {
- TRACE("%s(%s) has a higher version installed, skipping...",
-      img->id.name,
-      img->id.version);
-
- return true;
- }
- }
-
- return false;
-}
-
-/*
- * Remove the image if the same version is already installed
- */
-static void remove_installed_image_list(struct imglist *img_list,
- struct swver *sw_ver_list)
-{
- struct img_type *img, *tmp;
-
- LIST_FOREACH_SAFE(img, img_list, next, tmp) {
- if (is_image_installed(sw_ver_list, img)) {
- LIST_REMOVE(img, next);
- free_image(img);
- }
- }
-}
-
-/*
- * Remove the image if a higher version is already installed
- */
-static void remove_higher_image_list(struct imglist *img_list,
- struct swver *sw_ver_list)
-{
- struct img_type *img, *tmp;
-
- LIST_FOREACH_SAFE(img, img_list, next, tmp) {
- if (is_image_higher(sw_ver_list, img)) {
- LIST_REMOVE(img, next);
- free_image(img);
- }
- }
-}
-
 int parse(struct swupdate_cfg *sw, const char *descfile)
 {
  int ret = -1;
@@ -326,10 +232,6 @@ int parse(struct swupdate_cfg *sw, const char
*descfile)
  }
  }

- remove_installed_image_list(&sw->images, &sw->installed_sw_list);
-
- remove_higher_image_list(&sw->images, &sw->installed_sw_list);
-
  /*
  * Compute the total number of installer
  * to initialize the progress bar
diff --git a/core/stream_interface.c b/core/stream_interface.c
index 99e5c62..228c78a 100644
--- a/core/stream_interface.c
+++ b/core/stream_interface.c
@@ -281,7 +281,7 @@ static int extract_files(int fd, struct swupdate_cfg
*software)
  */

  LIST_FOREACH(img, &software->images, next) {
- if (! img->required)
+ if (  img->skip)
  continue;
  if (! img->fname[0])
  continue;
diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
index eb05429..2025942 100644
--- a/corelib/lua_interface.c
+++ b/corelib/lua_interface.c
@@ -336,6 +336,8 @@ static void lua_number_to_img(struct img_type *img,
const char *key,
  img->size = (long long)val;
  if (!strcmp(key, "checksum"))
  img->checksum = (unsigned int)val;
+ if (!strcmp(key, "skip"))
+ img->skip = (unsigned int)val;
 }

 #ifdef CONFIG_HANDLER_IN_LUA
@@ -492,6 +494,7 @@ static void update_table(lua_State* L, struct img_type
*img)
  LUA_PUSH_IMG_NUMBER(img, "offset", seek);
  LUA_PUSH_IMG_NUMBER(img, "size", size);
  LUA_PUSH_IMG_NUMBER(img, "checksum", checksum);
+ LUA_PUSH_IMG_NUMBER(img, "skip", skip);

  switch (img->compressed) {
  case COMPRESSED_ZLIB:
@@ -540,7 +543,7 @@ static void update_table(lua_State* L, struct img_type
*img)
 #endif

  lua_getfield(L, -1, "_private");
- LUA_PUSH_IMG_NUMBER(img, "offset", offset);
+        LUA_PUSH_IMG_NUMBER(img, "offset", offset);
  lua_pop(L, 1);

  char *hashstring = alloca(2 * SHA256_HASH_LENGTH + 1);
diff --git a/include/swupdate.h b/include/swupdate.h
index 8854d2f..29b756d 100644
--- a/include/swupdate.h
+++ b/include/swupdate.h
@@ -41,6 +41,13 @@ enum {
  INSTALL_FROM_STREAM
 };

+typedef enum {
+ SKIP_NONE=0,
+ SKIP_SAME,
+ SKIP_HIGHER,
+ SKIP_SCRIPT
+} skip_t;
+
 enum {
   COMPRESSED_FALSE,
   COMPRESSED_TRUE,
@@ -70,7 +77,7 @@ struct img_type {
  char extract_file[MAX_IMAGE_FNAME];
  char filesystem[MAX_IMAGE_FNAME];
  unsigned long long seek;
- int required;
+ skip_t skip;
  int provided;
  int compressed;
  int preserve_attributes; /* whether to preserve attributes in archives */
diff --git a/parser/parser.c b/parser/parser.c
index dce5180..8f8b25c 100644
--- a/parser/parser.c
+++ b/parser/parser.c
@@ -248,6 +248,68 @@ static int parse_hw_compatibility(parsertype
__attribute__ ((__unused__))p,
 }
 #endif

+static int is_image_installed(struct swver *sw_ver_list,
+                              struct img_type *img)
+{
+    struct sw_version *swver;
+
+    if (!sw_ver_list)
+        return false;
+
+    if (!strlen(img->id.name) || !strlen(img->id.version) ||
+        !img->id.install_if_different)
+        return false;
+
+    LIST_FOREACH(swver, sw_ver_list, next) {
+        /*
+         * Check if name and version are identical
+         */
+        if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) &&
+            !compare_versions(img->id.version, swver->version)) {
+            TRACE("%s(%s) already installed, skipping...",
+                  img->id.name,
+                  img->id.version);
+
+            return true;
+        }
+    }
+
+    return false;
+}
+
+static int is_image_higher(struct swver *sw_ver_list,
+                           struct img_type *img)
+{
+    struct sw_version *swver;
+
+    if (!sw_ver_list)
+        return false;
+
+    if (!strlen(img->id.name) || !strlen(img->id.version) ||
+        !img->id.install_if_higher)
+        return false;
+
+    LIST_FOREACH(swver, sw_ver_list, next) {
+        const char* current_version = swver->version;
+        const char* proposed_version = img->id.version;
+
+        /*
+         * Check if name are identical and the new version is lower
+         * or equal.
+         */
+        if (!strncmp(img->id.name, swver->name, sizeof(img->id.name)) &&
+            (compare_versions(proposed_version, current_version) < 0)) {
+            TRACE("%s(%s) has a higher version installed, skipping...",
+                  img->id.name,
+                  img->id.version);
+
+            return true;
+        }
+    }
+
+    return false;
+}
+
 static int run_embscript(parsertype p, void *elem, struct img_type *img,
  lua_State *L, const char *embscript)
 {
@@ -260,7 +322,7 @@ static int run_embscript(parsertype p, void *elem,
struct img_type *img,
  return lua_parser_fn(L, embfcn, img);
 }

-static int parse_common_attributes(parsertype p, void *elem, struct
img_type *image)
+static int parse_common_attributes(parsertype p, void *elem, struct
img_type *image, struct swupdate_cfg *cfg)
 {
  char seek_str[MAX_SEEK_STRING_SIZE];
  const char* compressed;
@@ -310,6 +372,14 @@ static int parse_common_attributes(parsertype p, void
*elem, struct img_type *im
  get_field(p, elem, "encrypted", &image->is_encrypted);
  GET_FIELD_STRING(p, elem, "ivt", image->ivt_ascii);

+ if (is_image_installed(&cfg->installed_sw_list, image)) {
+ image->skip = SKIP_SAME;
+ } else if (is_image_higher(&cfg->installed_sw_list, image)) {
+ image->skip = SKIP_HIGHER;
+ } else {
+ image->skip = SKIP_NONE;
+ }
+
  return 0;
 }

@@ -340,7 +410,7 @@ static int parse_partitions(parsertype p, void *cfg,
struct swupdate_cfg *swcfg)
  ERROR("No memory: malloc failed");
  return -ENOMEM;
  }
- if (parse_common_attributes(p, elem, partition) < 0) {
+ if (parse_common_attributes(p, elem, partition, swcfg) < 0) {
  free_image(partition);
  return -1;
  }
@@ -407,7 +477,7 @@ static int parse_scripts(parsertype p, void *cfg,
struct swupdate_cfg *swcfg, lu
  return -ENOMEM;
  }

- if (parse_common_attributes(p, elem, script) < 0) {
+ if (parse_common_attributes(p, elem, script, swcfg) < 0) {
  free_image(script);
  return -1;
  }
@@ -430,7 +500,7 @@ static int parse_scripts(parsertype p, void *cfg,
struct swupdate_cfg *swcfg, lu
  skip ? "Skip" : "Found",
  script->fname);

- if (skip) {
+ if (skip || script->skip != SKIP_NONE) {
  free_image(script);
  continue;
  }
@@ -500,7 +570,7 @@ static int parse_bootloader(parsertype p, void *cfg,
struct swupdate_cfg *swcfg,
  return -ENOMEM;
  }

- if (parse_common_attributes(p, elem, script) < 0) {
+ if (parse_common_attributes(p, elem, script, swcfg) < 0) {
  free_image(script);
  return -1;
  }
@@ -508,7 +578,7 @@ static int parse_bootloader(parsertype p, void *cfg,
struct swupdate_cfg *swcfg,
  script->is_script = 1;

  skip = run_embscript(p, elem, script, L, swcfg->embscript);
- if (skip != 0) {
+ if (skip != 0 || script->skip != SKIP_NONE) {
  free_image(script);
  if (skip < 0)
  return -1;
@@ -557,7 +627,7 @@ static int parse_images(parsertype p, void *cfg, struct
swupdate_cfg *swcfg, lua
  return -ENOMEM;
  }

- if (parse_common_attributes(p, elem, image) < 0) {
+ if (parse_common_attributes(p, elem, image, swcfg) < 0) {
  free_image(image);
  return -1;
  }
@@ -595,8 +665,7 @@ static int parse_images(parsertype p, void *cfg, struct
swupdate_cfg *swcfg, lua
     image->id.install_if_higher)) ?
  " Version must be checked" : ""
  );
-
- if (skip) {
+ if (skip || image->skip != SKIP_NONE) {
  free_image(image);
  continue;
  }
@@ -640,7 +709,7 @@ static int parse_files(parsertype p, void *cfg, struct
swupdate_cfg *swcfg, lua_
  return -ENOMEM;
  }

- if (parse_common_attributes(p, elem, file) < 0) {
+ if (parse_common_attributes(p, elem, file, swcfg) < 0) {
  free_image(file);
  return -1;
  }
@@ -670,7 +739,7 @@ static int parse_files(parsertype p, void *cfg, struct
swupdate_cfg *swcfg, lua_
  (strlen(file->id.name) && file->id.install_if_different) ?
  "; Version must be checked" : "");

- if (skip) {
+ if (skip || file->skip != SKIP_NONE) {
  free_image(file);
  continue;
  }

Hi Mauro,

patch is quite straightforward - I have just some small questions:

On 30.08.20 12:52, Mauro Condarelli wrote:
> Here it comes ;)
> Sorry for the mishap.
> I didn't use `git send-email` becauseI wanted to keep to this thread.
> Next version (with more implementation) I will switch to git.
> 
> Regards
> Mauro
> 
> From 8279090b0268d7efac63b5cb9bf8d08ebf68c529 Mon Sep 17 00:00:00 2001
> From: Mauro Condarelli <mc5686@mclink.it <mailto:mc5686@mclink.it>>
> Date: Fri, 28 Aug 2020 15:36:20 +0200
> Subject: [PATCH] Move artifact version check to inner parsing loop.
> 
> Artifact version, when install-if-differet/install-if-higher is specified,
> is now done before call to parsing hook (if any).
> 
> To do this:
> - functions is_image_installed() and is_image_higher() moved from
> core/parser.c
>   to parser/parser.c
> - calls to these functions, together with loops in
> remove_installed_image_list()
>   and remove_higher_image_list() were completely removed from core/parser.c
> - boolean `required` in `struct img_type` was replaced with `enum skip_t
> skip`
> - field `img_type.skip` is assigned a value detailing reason for skip
> (if any)
> - field `img_type.skip` is inserted into table passed to Lua hook (if
> any) end
>   retrieved after Lua call so hook function can override setting.
> - if `img_type.skip` is non-zero the image is considered "skippable" and the
>   whole `img_type` structure is removed from handling list.
> 
> Signed-off-by: Mauro Condarelli <mc5686@mclink.it <mailto:mc5686@mclink.it>>
> ---
>  core/parser.c           | 98 -----------------------------------------
>  core/stream_interface.c |  2 +-
>  corelib/lua_interface.c |  5 ++-
>  include/swupdate.h      |  9 +++-
>  parser/parser.c         | 91 +++++++++++++++++++++++++++++++++-----
>  5 files changed, 93 insertions(+), 112 deletions(-)
> 
> diff --git a/core/parser.c b/core/parser.c
> index 7f281cf..bc13d26 100644
> --- a/core/parser.c
> +++ b/core/parser.c
> @@ -130,100 +130,6 @@ static int check_handler_list(struct imglist *list,
>   return 0;
>  }
>  
> -static int is_image_installed(struct swver *sw_ver_list,
> - struct img_type *img)
> -{
> - struct sw_version *swver;
> -
> - if (!sw_ver_list)
> - return false;
> -
> - if (!strlen(img->id.name <http://id.name>) || !strlen(img->id.version) ||
> - !img->id.install_if_different)
> - return false;
> -
> - LIST_FOREACH(swver, sw_ver_list, next) {
> - /*
> - * Check if name and version are identical
> - */
> - if (!strncmp(img->id.name <http://id.name>, swver->name,
> sizeof(img->id.name <http://id.name>)) &&
> -    !compare_versions(img->id.version, swver->version)) {
> - TRACE("%s(%s) already installed, skipping...",
> - img->id.name <http://id.name>,
> - img->id.version);
> -
> - return true;
> - }
> - }
> -
> - return false;
> -}
> -
> -static int is_image_higher(struct swver *sw_ver_list,
> - struct img_type *img)
> -{
> - struct sw_version *swver;
> -
> - if (!sw_ver_list)
> - return false;
> -
> - if (!strlen(img->id.name <http://id.name>) || !strlen(img->id.version) ||
> - !img->id.install_if_higher)
> - return false;
> -
> - LIST_FOREACH(swver, sw_ver_list, next) {
> - const char* current_version = swver->version;
> - const char* proposed_version = img->id.version;
> -
> - /*
> - * Check if name are identical and the new version is lower
> - * or equal.
> - */
> - if (!strncmp(img->id.name <http://id.name>, swver->name,
> sizeof(img->id.name <http://id.name>)) &&
> -    (compare_versions(proposed_version, current_version) < 0)) {
> - TRACE("%s(%s) has a higher version installed, skipping...",
> -      img->id.name <http://id.name>,
> -      img->id.version);
> -
> - return true;
> - }
> - }
> -
> - return false;
> -}
> -
> -/*
> - * Remove the image if the same version is already installed
> - */
> -static void remove_installed_image_list(struct imglist *img_list,
> - struct swver *sw_ver_list)
> -{
> - struct img_type *img, *tmp;
> -
> - LIST_FOREACH_SAFE(img, img_list, next, tmp) {
> - if (is_image_installed(sw_ver_list, img)) {
> - LIST_REMOVE(img, next);
> - free_image(img);
> - }
> - }
> -}
> -
> -/*
> - * Remove the image if a higher version is already installed
> - */
> -static void remove_higher_image_list(struct imglist *img_list,
> - struct swver *sw_ver_list)
> -{
> - struct img_type *img, *tmp;
> -
> - LIST_FOREACH_SAFE(img, img_list, next, tmp) {
> - if (is_image_higher(sw_ver_list, img)) {
> - LIST_REMOVE(img, next);
> - free_image(img);
> - }
> - }
> -}
> -
>  int parse(struct swupdate_cfg *sw, const char *descfile)
>  {
>   int ret = -1;
> @@ -326,10 +232,6 @@ int parse(struct swupdate_cfg *sw, const char
> *descfile)
>   }
>   }
>  
> - remove_installed_image_list(&sw->images, &sw->installed_sw_list);
> -
> - remove_higher_image_list(&sw->images, &sw->installed_sw_list);
> -
>   /*
>   * Compute the total number of installer
>   * to initialize the progress bar
> diff --git a/core/stream_interface.c b/core/stream_interface.c
> index 99e5c62..228c78a 100644
> --- a/core/stream_interface.c
> +++ b/core/stream_interface.c
> @@ -281,7 +281,7 @@ static int extract_files(int fd, struct swupdate_cfg
> *software)
>   */
>  
>   LIST_FOREACH(img, &software->images, next) {
> - if (! img->required)
> + if (  img->skip)
>   continue;
>   if (! img->fname[0])
>   continue;
> diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> index eb05429..2025942 100644
> --- a/corelib/lua_interface.c
> +++ b/corelib/lua_interface.c
> @@ -336,6 +336,8 @@ static void lua_number_to_img(struct img_type *img,
> const char *key,
>   img->size = (long long)val;
>   if (!strcmp(key, "checksum"))
>   img->checksum = (unsigned int)val;
> + if (!strcmp(key, "skip"))
> + img->skip = (unsigned int)val;
>  }
>  
>  #ifdef CONFIG_HANDLER_IN_LUA
> @@ -492,6 +494,7 @@ static void update_table(lua_State* L, struct
> img_type *img)
>   LUA_PUSH_IMG_NUMBER(img, "offset", seek);
>   LUA_PUSH_IMG_NUMBER(img, "size", size);
>   LUA_PUSH_IMG_NUMBER(img, "checksum", checksum);
> + LUA_PUSH_IMG_NUMBER(img, "skip", skip);
>  
>   switch (img->compressed) {
>   case COMPRESSED_ZLIB:
> @@ -540,7 +543,7 @@ static void update_table(lua_State* L, struct
> img_type *img)
>  #endif
>  
>   lua_getfield(L, -1, "_private");
> - LUA_PUSH_IMG_NUMBER(img, "offset", offset);
> +        LUA_PUSH_IMG_NUMBER(img, "offset", offset);
>   lua_pop(L, 1);
>  
>   char *hashstring = alloca(2 * SHA256_HASH_LENGTH + 1);
> diff --git a/include/swupdate.h b/include/swupdate.h
> index 8854d2f..29b756d 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -41,6 +41,13 @@ enum {
>   INSTALL_FROM_STREAM
>  };
>  
> +typedef enum {
> + SKIP_NONE=0,
> + SKIP_SAME,
> + SKIP_HIGHER,
> + SKIP_SCRIPT
> +} skip_t;
> +
>  enum {
>    COMPRESSED_FALSE,
>    COMPRESSED_TRUE,
> @@ -70,7 +77,7 @@ struct img_type {
>   char extract_file[MAX_IMAGE_FNAME];
>   char filesystem[MAX_IMAGE_FNAME];
>   unsigned long long seek;
> - int required;
> + skip_t skip;
>   int provided;
>   int compressed;
>   int preserve_attributes; /* whether to preserve attributes in archives */
> diff --git a/parser/parser.c b/parser/parser.c
> index dce5180..8f8b25c 100644
> --- a/parser/parser.c
> +++ b/parser/parser.c
> @@ -248,6 +248,68 @@ static int parse_hw_compatibility(parsertype
> __attribute__ ((__unused__))p,
>  }
>  #endif
>  
> +static int is_image_installed(struct swver *sw_ver_list,
> +                              struct img_type *img)
> +{
> +    struct sw_version *swver;
> +
> +    if (!sw_ver_list)
> +        return false;
> +
> +    if (!strlen(img->id.name <http://id.name>) ||
> !strlen(img->id.version) ||
> +        !img->id.install_if_different)
> +        return false;
> +
> +    LIST_FOREACH(swver, sw_ver_list, next) {
> +        /*
> +         * Check if name and version are identical
> +         */
> +        if (!strncmp(img->id.name <http://id.name>, swver->name,
> sizeof(img->id.name <http://id.name>)) &&
> +            !compare_versions(img->id.version, swver->version)) {
> +            TRACE("%s(%s) already installed, skipping...",
> +                  img->id.name <http://id.name>,
> +                  img->id.version);
> +
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static int is_image_higher(struct swver *sw_ver_list,
> +                           struct img_type *img)
> +{
> +    struct sw_version *swver;
> +
> +    if (!sw_ver_list)
> +        return false;
> +
> +    if (!strlen(img->id.name <http://id.name>) ||
> !strlen(img->id.version) ||
> +        !img->id.install_if_higher)
> +        return false;
> +
> +    LIST_FOREACH(swver, sw_ver_list, next) {
> +        const char* current_version = swver->version;
> +        const char* proposed_version = img->id.version;
> +
> +        /*
> +         * Check if name are identical and the new version is lower
> +         * or equal.
> +         */
> +        if (!strncmp(img->id.name <http://id.name>, swver->name,
> sizeof(img->id.name <http://id.name>)) &&
> +            (compare_versions(proposed_version, current_version) < 0)) {
> +            TRACE("%s(%s) has a higher version installed, skipping...",
> +                  img->id.name <http://id.name>,
> +                  img->id.version);
> +
> +            return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  static int run_embscript(parsertype p, void *elem, struct img_type *img,
>   lua_State *L, const char *embscript)
>  {
> @@ -260,7 +322,7 @@ static int run_embscript(parsertype p, void *elem,
> struct img_type *img,
>   return lua_parser_fn(L, embfcn, img);
>  }
>  
> -static int parse_common_attributes(parsertype p, void *elem, struct
> img_type *image)
> +static int parse_common_attributes(parsertype p, void *elem, struct
> img_type *image, struct swupdate_cfg *cfg)
>  {
>   char seek_str[MAX_SEEK_STRING_SIZE];
>   const char* compressed;
> @@ -310,6 +372,14 @@ static int parse_common_attributes(parsertype p,
> void *elem, struct img_type *im
>   get_field(p, elem, "encrypted", &image->is_encrypted);
>   GET_FIELD_STRING(p, elem, "ivt", image->ivt_ascii);
>  
> + if (is_image_installed(&cfg->installed_sw_list, image)) {
> + image->skip = SKIP_SAME;
> + } else if (is_image_higher(&cfg->installed_sw_list, image)) {
> + image->skip = SKIP_HIGHER;
> + } else {
> + image->skip = SKIP_NONE;
> + }

I have no problem to replace the boolean "required" with the enum
"skip". I just ask why do we need to store the reason: due to some
rules, an artefact is skipped. Does it matter if it is skipped because
the same version is installed or if a newer must be installed ? Someone
in code should then evaluate img->skip and do different things, but
SWUpdate just does not install it if it is not SKIP_NONE.

> +
>   return 0;
>  }
>  
> @@ -340,7 +410,7 @@ static int parse_partitions(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg)
>   ERROR("No memory: malloc failed");
>   return -ENOMEM;
>   }
> - if (parse_common_attributes(p, elem, partition) < 0) {
> + if (parse_common_attributes(p, elem, partition, swcfg) < 0) {
>   free_image(partition);
>   return -1;
>   }
> @@ -407,7 +477,7 @@ static int parse_scripts(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lu
>   return -ENOMEM;
>   }
>  
> - if (parse_common_attributes(p, elem, script) < 0) {
> + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
>   free_image(script);
>   return -1;
>   }
> @@ -430,7 +500,7 @@ static int parse_scripts(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lu
>   skip ? "Skip" : "Found",
>   script->fname);
>  
> - if (skip) {
> + if (skip || script->skip != SKIP_NONE) {
>   free_image(script);
>   continue;
>   }
> @@ -500,7 +570,7 @@ static int parse_bootloader(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg,
>   return -ENOMEM;
>   }
>  
> - if (parse_common_attributes(p, elem, script) < 0) {
> + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
>   free_image(script);
>   return -1;
>   }
> @@ -508,7 +578,7 @@ static int parse_bootloader(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg,
>   script->is_script = 1;
>  
>   skip = run_embscript(p, elem, script, L, swcfg->embscript);
> - if (skip != 0) {
> + if (skip != 0 || script->skip != SKIP_NONE) {
>   free_image(script);
>   if (skip < 0)
>   return -1;
> @@ -557,7 +627,7 @@ static int parse_images(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lua
>   return -ENOMEM;
>   }
>  
> - if (parse_common_attributes(p, elem, image) < 0) {
> + if (parse_common_attributes(p, elem, image, swcfg) < 0) {
>   free_image(image);
>   return -1;
>   }
> @@ -595,8 +665,7 @@ static int parse_images(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lua
>      image->id.install_if_higher)) ?
>   " Version must be checked" : ""
>   );
> -
> - if (skip) {
> + if (skip || image->skip != SKIP_NONE) {
>   free_image(image);
>   continue;
>   }
> @@ -640,7 +709,7 @@ static int parse_files(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lua_
>   return -ENOMEM;
>   }
>  
> - if (parse_common_attributes(p, elem, file) < 0) {
> + if (parse_common_attributes(p, elem, file, swcfg) < 0) {
>   free_image(file);
>   return -1;
>   }
> @@ -670,7 +739,7 @@ static int parse_files(parsertype p, void *cfg,
> struct swupdate_cfg *swcfg, lua_
>   (strlen(file->id.name <http://id.name>) &&
> file->id.install_if_different) ?
>   "; Version must be checked" : "");
>  
> - if (skip) {
> + if (skip || file->skip != SKIP_NONE) {
>   free_image(file);
>   continue;
>   }
> -- 
> 2.25.1
> 

Els eI do not see issue in the patch, after some testing it could be
applied.

Best regards,
Stefano Babic

> 
> 
> On Sat, Aug 29, 2020 at 10:47 AM Stefano Babic <sbabic@denx.de
> <mailto:sbabic@denx.de>> wrote:
> 
>     Hi Mauro,
> 
>     can't you send the patch inline (as you did before), maybe using git
>     send-email ? It is difficult to add comments if the patch is attached.
> 
>     Best regards,
>     Stefano Babic
> 
>     On 28.08.20 16:26, Mauro Condarelli wrote:
>     > Hi,
>     > this is the first patch of a series (see below) meant to enhance Hook
>     > handling.
>     >
>     > Commit message reads:
>     >
>     >          Move artifact version check to inner parsing loop.
>     >
>     >          Artifact version, when
>     install-if-differet/install-if-higher is
>     >     specified,
>     >          is now done before call to parsing hook (if any).
>     >
>     >          To do this:
>     >          - functions is_image_installed() and is_image_higher() moved
>     >     from core/parser.c
>     >            to parser/parser.c
>     >          - calls to these functions, together with loops in
>     >     remove_installed_image_list()
>     >            and remove_higher_image_list() were completely removed from
>     >     core/parser.c
>     >          - boolean `required` in `struct img_type` was replaced with
>     >     `enum skip_t skip`
>     >          - field `img_type.skip` is assigned a value detailing reason
>     >     for skip (if any)
>     >          - field `img_type.skip` is inserted into table passed to Lua
>     >     hook (if any) end
>     >            retrieved after Lua call so hook function can override
>     setting.
>     >          - if `img_type.skip` is non-zero the image is considered
>     >     "skippable" and the
>     >            whole `img_type` structure is removed from handling list.
>     >
>     >          Signed-off-by: Mauro Condarelli <mc5686@mclink.it
>     <mailto:mc5686@mclink.it>
>     >     <mailto:mc5686@mclink.it <mailto:mc5686@mclink.it>>>
>     >
>     >
>     > Next patches will:
>     >
>     >  1. make `embedded-script` Lua environment "permanent" (i.e.: not
>     >     constrained to parsing).
>     >  2. add `pre-install-hook` and `post-install-hook` to artifacts.
>     >  3. ensure these hooks are called at the appropriate moment regardless
>     >     of `installed-directly` setting.
>     >  4. add a series of standard hooks (to be defined in
>     `embedded-script`)
>     >     to be called at specific points; e.g.: startup, before-parsing,
>     >     after-parsing, before-installation, after-installation,
>     >     before-bootvars, after-bootvars, on-error.
>     >  5. all  above hooks will be defined in `embedded-script`and they will
>     >     share a single environment, so that variables set in one hook will
>     >     be available to all hooks called after that.
>     >
>     > As always comments and criticism are most welcome.
>     >
>     > Regards
>     > Mauro
>     >
>     > --
>     > You received this message because you are subscribed to the Google
>     > Groups "swupdate" group.
>     > To unsubscribe from this group and stop receiving emails from it,
>     send
>     > an email to swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>
>     > <mailto:swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>>.
>     > To view this discussion on the web visit
>     >
>     https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com
> 
>     >
>     <https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com?utm_medium=email&utm_source=footer>.
> 
>     -- 
>     =====================================================================
>     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>     Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email:
>     sbabic@denx.de <mailto:sbabic@denx.de>
>     =====================================================================
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+unsubscribe@googlegroups.com
> <mailto:swupdate+unsubscribe@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com
> <https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com?utm_medium=email&utm_source=footer>.

Hi Stefano,
comments inline below.

On Wed, Sep 2, 2020 at 3:06 PM Stefano Babic <sbabic@denx.de> wrote:

> Hi Mauro,
>
> patch is quite straightforward - I have just some small questions:
>
> On 30.08.20 12:52, Mauro Condarelli wrote:
> > Here it comes ;)
> > Sorry for the mishap.
> > I didn't use `git send-email` becauseI wanted to keep to this thread.
> > Next version (with more implementation) I will switch to git.
> >
> > Regards
> > Mauro
> >
> > From 8279090b0268d7efac63b5cb9bf8d08ebf68c529 Mon Sep 17 00:00:00 2001
> > From: Mauro Condarelli <mc5686@mclink.it <mailto:mc5686@mclink.it>>
> > Date: Fri, 28 Aug 2020 15:36:20 +0200
> > Subject: [PATCH] Move artifact version check to inner parsing loop.
> >
> > Artifact version, when install-if-differet/install-if-higher is
> specified,
> > is now done before call to parsing hook (if any).
> >
> > To do this:
> > - functions is_image_installed() and is_image_higher() moved from
> > core/parser.c
> >   to parser/parser.c
> > - calls to these functions, together with loops in
> > remove_installed_image_list()
> >   and remove_higher_image_list() were completely removed from
> core/parser.c
> > - boolean `required` in `struct img_type` was replaced with `enum skip_t
> > skip`
> > - field `img_type.skip` is assigned a value detailing reason for skip
> > (if any)
> > - field `img_type.skip` is inserted into table passed to Lua hook (if
> > any) end
> >   retrieved after Lua call so hook function can override setting.
> > - if `img_type.skip` is non-zero the image is considered "skippable" and
> the
> >   whole `img_type` structure is removed from handling list.
> >
> > Signed-off-by: Mauro Condarelli <mc5686@mclink.it <mailto:
> mc5686@mclink.it>>
> > ---
> >  core/parser.c           | 98 -----------------------------------------
> >  core/stream_interface.c |  2 +-
> >  corelib/lua_interface.c |  5 ++-
> >  include/swupdate.h      |  9 +++-
> >  parser/parser.c         | 91 +++++++++++++++++++++++++++++++++-----
> >  5 files changed, 93 insertions(+), 112 deletions(-)
> >
> > diff --git a/core/parser.c b/core/parser.c
> > index 7f281cf..bc13d26 100644
> > --- a/core/parser.c
> > +++ b/core/parser.c
> > @@ -130,100 +130,6 @@ static int check_handler_list(struct imglist *list,
> >   return 0;
> >  }
> >
> > -static int is_image_installed(struct swver *sw_ver_list,
> > - struct img_type *img)
> > -{
> > - struct sw_version *swver;
> > -
> > - if (!sw_ver_list)
> > - return false;
> > -
> > - if (!strlen(img->id.name <http://id.name>) ||
> !strlen(img->id.version) ||
> > - !img->id.install_if_different)
> > - return false;
> > -
> > - LIST_FOREACH(swver, sw_ver_list, next) {
> > - /*
> > - * Check if name and version are identical
> > - */
> > - if (!strncmp(img->id.name <http://id.name>, swver->name,
> > sizeof(img->id.name <http://id.name>)) &&
> > -    !compare_versions(img->id.version, swver->version)) {
> > - TRACE("%s(%s) already installed, skipping...",
> > - img->id.name <http://id.name>,
> > - img->id.version);
> > -
> > - return true;
> > - }
> > - }
> > -
> > - return false;
> > -}
> > -
> > -static int is_image_higher(struct swver *sw_ver_list,
> > - struct img_type *img)
> > -{
> > - struct sw_version *swver;
> > -
> > - if (!sw_ver_list)
> > - return false;
> > -
> > - if (!strlen(img->id.name <http://id.name>) ||
> !strlen(img->id.version) ||
> > - !img->id.install_if_higher)
> > - return false;
> > -
> > - LIST_FOREACH(swver, sw_ver_list, next) {
> > - const char* current_version = swver->version;
> > - const char* proposed_version = img->id.version;
> > -
> > - /*
> > - * Check if name are identical and the new version is lower
> > - * or equal.
> > - */
> > - if (!strncmp(img->id.name <http://id.name>, swver->name,
> > sizeof(img->id.name <http://id.name>)) &&
> > -    (compare_versions(proposed_version, current_version) < 0)) {
> > - TRACE("%s(%s) has a higher version installed, skipping...",
> > -      img->id.name <http://id.name>,
> > -      img->id.version);
> > -
> > - return true;
> > - }
> > - }
> > -
> > - return false;
> > -}
> > -
> > -/*
> > - * Remove the image if the same version is already installed
> > - */
> > -static void remove_installed_image_list(struct imglist *img_list,
> > - struct swver *sw_ver_list)
> > -{
> > - struct img_type *img, *tmp;
> > -
> > - LIST_FOREACH_SAFE(img, img_list, next, tmp) {
> > - if (is_image_installed(sw_ver_list, img)) {
> > - LIST_REMOVE(img, next);
> > - free_image(img);
> > - }
> > - }
> > -}
> > -
> > -/*
> > - * Remove the image if a higher version is already installed
> > - */
> > -static void remove_higher_image_list(struct imglist *img_list,
> > - struct swver *sw_ver_list)
> > -{
> > - struct img_type *img, *tmp;
> > -
> > - LIST_FOREACH_SAFE(img, img_list, next, tmp) {
> > - if (is_image_higher(sw_ver_list, img)) {
> > - LIST_REMOVE(img, next);
> > - free_image(img);
> > - }
> > - }
> > -}
> > -
> >  int parse(struct swupdate_cfg *sw, const char *descfile)
> >  {
> >   int ret = -1;
> > @@ -326,10 +232,6 @@ int parse(struct swupdate_cfg *sw, const char
> > *descfile)
> >   }
> >   }
> >
> > - remove_installed_image_list(&sw->images, &sw->installed_sw_list);
> > -
> > - remove_higher_image_list(&sw->images, &sw->installed_sw_list);
> > -
> >   /*
> >   * Compute the total number of installer
> >   * to initialize the progress bar
> > diff --git a/core/stream_interface.c b/core/stream_interface.c
> > index 99e5c62..228c78a 100644
> > --- a/core/stream_interface.c
> > +++ b/core/stream_interface.c
> > @@ -281,7 +281,7 @@ static int extract_files(int fd, struct swupdate_cfg
> > *software)
> >   */
> >
> >   LIST_FOREACH(img, &software->images, next) {
> > - if (! img->required)
> > + if (  img->skip)
> >   continue;
> >   if (! img->fname[0])
> >   continue;
> > diff --git a/corelib/lua_interface.c b/corelib/lua_interface.c
> > index eb05429..2025942 100644
> > --- a/corelib/lua_interface.c
> > +++ b/corelib/lua_interface.c
> > @@ -336,6 +336,8 @@ static void lua_number_to_img(struct img_type *img,
> > const char *key,
> >   img->size = (long long)val;
> >   if (!strcmp(key, "checksum"))
> >   img->checksum = (unsigned int)val;
> > + if (!strcmp(key, "skip"))
> > + img->skip = (unsigned int)val;
> >  }
> >
> >  #ifdef CONFIG_HANDLER_IN_LUA
> > @@ -492,6 +494,7 @@ static void update_table(lua_State* L, struct
> > img_type *img)
> >   LUA_PUSH_IMG_NUMBER(img, "offset", seek);
> >   LUA_PUSH_IMG_NUMBER(img, "size", size);
> >   LUA_PUSH_IMG_NUMBER(img, "checksum", checksum);
> > + LUA_PUSH_IMG_NUMBER(img, "skip", skip);
> >
> >   switch (img->compressed) {
> >   case COMPRESSED_ZLIB:
> > @@ -540,7 +543,7 @@ static void update_table(lua_State* L, struct
> > img_type *img)
> >  #endif
> >
> >   lua_getfield(L, -1, "_private");
> > - LUA_PUSH_IMG_NUMBER(img, "offset", offset);
> > +        LUA_PUSH_IMG_NUMBER(img, "offset", offset);
> >   lua_pop(L, 1);
> >
> >   char *hashstring = alloca(2 * SHA256_HASH_LENGTH + 1);
> > diff --git a/include/swupdate.h b/include/swupdate.h
> > index 8854d2f..29b756d 100644
> > --- a/include/swupdate.h
> > +++ b/include/swupdate.h
> > @@ -41,6 +41,13 @@ enum {
> >   INSTALL_FROM_STREAM
> >  };
> >
> > +typedef enum {
> > + SKIP_NONE=0,
> > + SKIP_SAME,
> > + SKIP_HIGHER,
> > + SKIP_SCRIPT
> > +} skip_t;
> > +
> >  enum {
> >    COMPRESSED_FALSE,
> >    COMPRESSED_TRUE,
> > @@ -70,7 +77,7 @@ struct img_type {
> >   char extract_file[MAX_IMAGE_FNAME];
> >   char filesystem[MAX_IMAGE_FNAME];
> >   unsigned long long seek;
> > - int required;
> > + skip_t skip;
> >   int provided;
> >   int compressed;
> >   int preserve_attributes; /* whether to preserve attributes in archives
> */
> > diff --git a/parser/parser.c b/parser/parser.c
> > index dce5180..8f8b25c 100644
> > --- a/parser/parser.c
> > +++ b/parser/parser.c
> > @@ -248,6 +248,68 @@ static int parse_hw_compatibility(parsertype
> > __attribute__ ((__unused__))p,
> >  }
> >  #endif
> >
> > +static int is_image_installed(struct swver *sw_ver_list,
> > +                              struct img_type *img)
> > +{
> > +    struct sw_version *swver;
> > +
> > +    if (!sw_ver_list)
> > +        return false;
> > +
> > +    if (!strlen(img->id.name <http://id.name>) ||
> > !strlen(img->id.version) ||
> > +        !img->id.install_if_different)
> > +        return false;
> > +
> > +    LIST_FOREACH(swver, sw_ver_list, next) {
> > +        /*
> > +         * Check if name and version are identical
> > +         */
> > +        if (!strncmp(img->id.name <http://id.name>, swver->name,
> > sizeof(img->id.name <http://id.name>)) &&
> > +            !compare_versions(img->id.version, swver->version)) {
> > +            TRACE("%s(%s) already installed, skipping...",
> > +                  img->id.name <http://id.name>,
> > +                  img->id.version);
> > +
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +static int is_image_higher(struct swver *sw_ver_list,
> > +                           struct img_type *img)
> > +{
> > +    struct sw_version *swver;
> > +
> > +    if (!sw_ver_list)
> > +        return false;
> > +
> > +    if (!strlen(img->id.name <http://id.name>) ||
> > !strlen(img->id.version) ||
> > +        !img->id.install_if_higher)
> > +        return false;
> > +
> > +    LIST_FOREACH(swver, sw_ver_list, next) {
> > +        const char* current_version = swver->version;
> > +        const char* proposed_version = img->id.version;
> > +
> > +        /*
> > +         * Check if name are identical and the new version is lower
> > +         * or equal.
> > +         */
> > +        if (!strncmp(img->id.name <http://id.name>, swver->name,
> > sizeof(img->id.name <http://id.name>)) &&
> > +            (compare_versions(proposed_version, current_version) < 0)) {
> > +            TRACE("%s(%s) has a higher version installed, skipping...",
> > +                  img->id.name <http://id.name>,
> > +                  img->id.version);
> > +
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  static int run_embscript(parsertype p, void *elem, struct img_type *img,
> >   lua_State *L, const char *embscript)
> >  {
> > @@ -260,7 +322,7 @@ static int run_embscript(parsertype p, void *elem,
> > struct img_type *img,
> >   return lua_parser_fn(L, embfcn, img);
> >  }
> >
> > -static int parse_common_attributes(parsertype p, void *elem, struct
> > img_type *image)
> > +static int parse_common_attributes(parsertype p, void *elem, struct
> > img_type *image, struct swupdate_cfg *cfg)
> >  {
> >   char seek_str[MAX_SEEK_STRING_SIZE];
> >   const char* compressed;
> > @@ -310,6 +372,14 @@ static int parse_common_attributes(parsertype p,
> > void *elem, struct img_type *im
> >   get_field(p, elem, "encrypted", &image->is_encrypted);
> >   GET_FIELD_STRING(p, elem, "ivt", image->ivt_ascii);
> >
> > + if (is_image_installed(&cfg->installed_sw_list, image)) {
> > + image->skip = SKIP_SAME;
> > + } else if (is_image_higher(&cfg->installed_sw_list, image)) {
> > + image->skip = SKIP_HIGHER;
> > + } else {
> > + image->skip = SKIP_NONE;
> > + }
>
> I have no problem to replace the boolean "required" with the enum
> "skip". I just ask why do we need to store the reason: due to some
> rules, an artefact is skipped. Does it matter if it is skipped because
> the same version is installed or if a newer must be installed ? Someone
> in code should then evaluate img->skip and do different things, but
> SWUpdate just does not install it if it is not SKIP_NONE.
>
That is a just-in-case I don't actually plan to use.
Rationale is some Lua handler might differentiate based on reason for
skipping and selectively override (e.g.: this update needs *exactly* that
version because..., so leave it alone id it's SKIP_SAME, but override
if it's SKIP_HIGHER).
The Intent was to allow Lua handlers to decide.
I have no problem (in the foreseeable future) to revert to boolean.


>
> > +
> >   return 0;
> >  }
> >
> > @@ -340,7 +410,7 @@ static int parse_partitions(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg)
> >   ERROR("No memory: malloc failed");
> >   return -ENOMEM;
> >   }
> > - if (parse_common_attributes(p, elem, partition) < 0) {
> > + if (parse_common_attributes(p, elem, partition, swcfg) < 0) {
> >   free_image(partition);
> >   return -1;
> >   }
> > @@ -407,7 +477,7 @@ static int parse_scripts(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lu
> >   return -ENOMEM;
> >   }
> >
> > - if (parse_common_attributes(p, elem, script) < 0) {
> > + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
> >   free_image(script);
> >   return -1;
> >   }
> > @@ -430,7 +500,7 @@ static int parse_scripts(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lu
> >   skip ? "Skip" : "Found",
> >   script->fname);
> >
> > - if (skip) {
> > + if (skip || script->skip != SKIP_NONE) {
> >   free_image(script);
> >   continue;
> >   }
> > @@ -500,7 +570,7 @@ static int parse_bootloader(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg,
> >   return -ENOMEM;
> >   }
> >
> > - if (parse_common_attributes(p, elem, script) < 0) {
> > + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
> >   free_image(script);
> >   return -1;
> >   }
> > @@ -508,7 +578,7 @@ static int parse_bootloader(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg,
> >   script->is_script = 1;
> >
> >   skip = run_embscript(p, elem, script, L, swcfg->embscript);
> > - if (skip != 0) {
> > + if (skip != 0 || script->skip != SKIP_NONE) {
> >   free_image(script);
> >   if (skip < 0)
> >   return -1;
> > @@ -557,7 +627,7 @@ static int parse_images(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lua
> >   return -ENOMEM;
> >   }
> >
> > - if (parse_common_attributes(p, elem, image) < 0) {
> > + if (parse_common_attributes(p, elem, image, swcfg) < 0) {
> >   free_image(image);
> >   return -1;
> >   }
> > @@ -595,8 +665,7 @@ static int parse_images(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lua
> >      image->id.install_if_higher)) ?
> >   " Version must be checked" : ""
> >   );
> > -
> > - if (skip) {
> > + if (skip || image->skip != SKIP_NONE) {
> >   free_image(image);
> >   continue;
> >   }
> > @@ -640,7 +709,7 @@ static int parse_files(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lua_
> >   return -ENOMEM;
> >   }
> >
> > - if (parse_common_attributes(p, elem, file) < 0) {
> > + if (parse_common_attributes(p, elem, file, swcfg) < 0) {
> >   free_image(file);
> >   return -1;
> >   }
> > @@ -670,7 +739,7 @@ static int parse_files(parsertype p, void *cfg,
> > struct swupdate_cfg *swcfg, lua_
> >   (strlen(file->id.name <http://id.name>) &&
> > file->id.install_if_different) ?
> >   "; Version must be checked" : "");
> >
> > - if (skip) {
> > + if (skip || file->skip != SKIP_NONE) {
> >   free_image(file);
> >   continue;
> >   }
> > --
> > 2.25.1
> >
>
> Els eI do not see issue in the patch, after some testing it could be
> applied.
>
> Best regards,
> Stefano Babic
>
> >
> >
> > On Sat, Aug 29, 2020 at 10:47 AM Stefano Babic <sbabic@denx.de
> > <mailto:sbabic@denx.de>> wrote:
> >
> >     Hi Mauro,
> >
> >     can't you send the patch inline (as you did before), maybe using git
> >     send-email ? It is difficult to add comments if the patch is
> attached.
> >
> >     Best regards,
> >     Stefano Babic
> >
> >     On 28.08.20 16:26, Mauro Condarelli wrote:
> >     > Hi,
> >     > this is the first patch of a series (see below) meant to enhance
> Hook
> >     > handling.
> >     >
> >     > Commit message reads:
> >     >
> >     >          Move artifact version check to inner parsing loop.
> >     >
> >     >          Artifact version, when
> >     install-if-differet/install-if-higher is
> >     >     specified,
> >     >          is now done before call to parsing hook (if any).
> >     >
> >     >          To do this:
> >     >          - functions is_image_installed() and is_image_higher()
> moved
> >     >     from core/parser.c
> >     >            to parser/parser.c
> >     >          - calls to these functions, together with loops in
> >     >     remove_installed_image_list()
> >     >            and remove_higher_image_list() were completely removed
> from
> >     >     core/parser.c
> >     >          - boolean `required` in `struct img_type` was replaced
> with
> >     >     `enum skip_t skip`
> >     >          - field `img_type.skip` is assigned a value detailing
> reason
> >     >     for skip (if any)
> >     >          - field `img_type.skip` is inserted into table passed to
> Lua
> >     >     hook (if any) end
> >     >            retrieved after Lua call so hook function can override
> >     setting.
> >     >          - if `img_type.skip` is non-zero the image is considered
> >     >     "skippable" and the
> >     >            whole `img_type` structure is removed from handling
> list.
> >     >
> >     >          Signed-off-by: Mauro Condarelli <mc5686@mclink.it
> >     <mailto:mc5686@mclink.it>
> >     >     <mailto:mc5686@mclink.it <mailto:mc5686@mclink.it>>>
> >     >
> >     >
> >     > Next patches will:
> >     >
> >     >  1. make `embedded-script` Lua environment "permanent" (i.e.: not
> >     >     constrained to parsing).
> >     >  2. add `pre-install-hook` and `post-install-hook` to artifacts.
> >     >  3. ensure these hooks are called at the appropriate moment
> regardless
> >     >     of `installed-directly` setting.
> >     >  4. add a series of standard hooks (to be defined in
> >     `embedded-script`)
> >     >     to be called at specific points; e.g.: startup, before-parsing,
> >     >     after-parsing, before-installation, after-installation,
> >     >     before-bootvars, after-bootvars, on-error.
> >     >  5. all  above hooks will be defined in `embedded-script`and they
> will
> >     >     share a single environment, so that variables set in one hook
> will
> >     >     be available to all hooks called after that.
> >     >
> >     > As always comments and criticism are most welcome.
> >     >
> >     > Regards
> >     > Mauro
> >     >
> >     > --
> >     > You received this message because you are subscribed to the Google
> >     > Groups "swupdate" group.
> >     > To unsubscribe from this group and stop receiving emails from it,
> >     send
> >     > an email to swupdate+unsubscribe@googlegroups.com
> >     <mailto:swupdate%2Bunsubscribe@googlegroups.com>
> >     > <mailto:swupdate+unsubscribe@googlegroups.com
> >     <mailto:swupdate%2Bunsubscribe@googlegroups.com>>.
> >     > To view this discussion on the web visit
> >     >
> >
> https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com
> >
> >     >
> >     <
> https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com?utm_medium=email&utm_source=footer
> >.
> >
> >     --
> >     =====================================================================
> >     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> >     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> >     Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email:
> >     sbabic@denx.de <mailto:sbabic@denx.de>
> >     =====================================================================
> >
> > --
> > You received this message because you are subscribed to the Google
> > Groups "swupdate" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> > an email to swupdate+unsubscribe@googlegroups.com
> > <mailto:swupdate+unsubscribe@googlegroups.com>.
> > To view this discussion on the web visit
> >
> https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com
> > <
> https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com?utm_medium=email&utm_source=footer
> >.
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================
>

Hi Mauro,

On 03.09.20 11:20, Mauro Condarelli wrote:
> Hi Stefano,
> comments inline below.
> 

[snip]

>     I have no problem to replace the boolean "required" with the enum
>     "skip". I just ask why do we need to store the reason: due to some
>     rules, an artefact is skipped. Does it matter if it is skipped because
>     the same version is installed or if a newer must be installed ? Someone
>     in code should then evaluate img->skip and do different things, but
>     SWUpdate just does not install it if it is not SKIP_NONE.
> 
> That is a just-in-case I don't actually plan to use.
> Rationale is some Lua handler might differentiate based on reason for
> skipping and selectively override (e.g.: this update needs *exactly* that
> version because..., so leave it alone id it's SKIP_SAME, but override
> if it's SKIP_HIGHER).
> The Intent was to allow Lua handlers to decide.
> I have no problem (in the foreseeable future) to revert to boolean.

ok - I have also no problem until no there are no rgression issues. I
will try to test you pytch here and if I do not find any issue I will
merge it.

Regards,
Stefano Babic

>  
> 
> 
>     > +
>     >   return 0;
>     >  }
>     >  
>     > @@ -340,7 +410,7 @@ static int parse_partitions(parsertype p, void
>     *cfg,
>     > struct swupdate_cfg *swcfg)
>     >   ERROR("No memory: malloc failed");
>     >   return -ENOMEM;
>     >   }
>     > - if (parse_common_attributes(p, elem, partition) < 0) {
>     > + if (parse_common_attributes(p, elem, partition, swcfg) < 0) {
>     >   free_image(partition);
>     >   return -1;
>     >   }
>     > @@ -407,7 +477,7 @@ static int parse_scripts(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lu
>     >   return -ENOMEM;
>     >   }
>     >  
>     > - if (parse_common_attributes(p, elem, script) < 0) {
>     > + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
>     >   free_image(script);
>     >   return -1;
>     >   }
>     > @@ -430,7 +500,7 @@ static int parse_scripts(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lu
>     >   skip ? "Skip" : "Found",
>     >   script->fname);
>     >  
>     > - if (skip) {
>     > + if (skip || script->skip != SKIP_NONE) {
>     >   free_image(script);
>     >   continue;
>     >   }
>     > @@ -500,7 +570,7 @@ static int parse_bootloader(parsertype p, void
>     *cfg,
>     > struct swupdate_cfg *swcfg,
>     >   return -ENOMEM;
>     >   }
>     >  
>     > - if (parse_common_attributes(p, elem, script) < 0) {
>     > + if (parse_common_attributes(p, elem, script, swcfg) < 0) {
>     >   free_image(script);
>     >   return -1;
>     >   }
>     > @@ -508,7 +578,7 @@ static int parse_bootloader(parsertype p, void
>     *cfg,
>     > struct swupdate_cfg *swcfg,
>     >   script->is_script = 1;
>     >  
>     >   skip = run_embscript(p, elem, script, L, swcfg->embscript);
>     > - if (skip != 0) {
>     > + if (skip != 0 || script->skip != SKIP_NONE) {
>     >   free_image(script);
>     >   if (skip < 0)
>     >   return -1;
>     > @@ -557,7 +627,7 @@ static int parse_images(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lua
>     >   return -ENOMEM;
>     >   }
>     >  
>     > - if (parse_common_attributes(p, elem, image) < 0) {
>     > + if (parse_common_attributes(p, elem, image, swcfg) < 0) {
>     >   free_image(image);
>     >   return -1;
>     >   }
>     > @@ -595,8 +665,7 @@ static int parse_images(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lua
>     >      image->id.install_if_higher)) ?
>     >   " Version must be checked" : ""
>     >   );
>     > -
>     > - if (skip) {
>     > + if (skip || image->skip != SKIP_NONE) {
>     >   free_image(image);
>     >   continue;
>     >   }
>     > @@ -640,7 +709,7 @@ static int parse_files(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lua_
>     >   return -ENOMEM;
>     >   }
>     >  
>     > - if (parse_common_attributes(p, elem, file) < 0) {
>     > + if (parse_common_attributes(p, elem, file, swcfg) < 0) {
>     >   free_image(file);
>     >   return -1;
>     >   }
>     > @@ -670,7 +739,7 @@ static int parse_files(parsertype p, void *cfg,
>     > struct swupdate_cfg *swcfg, lua_
>     >   (strlen(file->id.name <http://id.name> <http://id.name>) &&
>     > file->id.install_if_different) ?
>     >   "; Version must be checked" : "");
>     >  
>     > - if (skip) {
>     > + if (skip || file->skip != SKIP_NONE) {
>     >   free_image(file);
>     >   continue;
>     >   }
>     > --
>     > 2.25.1
>     >
> 
>     Els eI do not see issue in the patch, after some testing it could be
>     applied.
> 
>     Best regards,
>     Stefano Babic
> 
>     >
>     >
>     > On Sat, Aug 29, 2020 at 10:47 AM Stefano Babic <sbabic@denx.de
>     <mailto:sbabic@denx.de>
>     > <mailto:sbabic@denx.de <mailto:sbabic@denx.de>>> wrote:
>     >
>     >     Hi Mauro,
>     >
>     >     can't you send the patch inline (as you did before), maybe
>     using git
>     >     send-email ? It is difficult to add comments if the patch is
>     attached.
>     >
>     >     Best regards,
>     >     Stefano Babic
>     >
>     >     On 28.08.20 16:26, Mauro Condarelli wrote:
>     >     > Hi,
>     >     > this is the first patch of a series (see below) meant to
>     enhance Hook
>     >     > handling.
>     >     >
>     >     > Commit message reads:
>     >     >
>     >     >          Move artifact version check to inner parsing loop.
>     >     >
>     >     >          Artifact version, when
>     >     install-if-differet/install-if-higher is
>     >     >     specified,
>     >     >          is now done before call to parsing hook (if any).
>     >     >
>     >     >          To do this:
>     >     >          - functions is_image_installed() and
>     is_image_higher() moved
>     >     >     from core/parser.c
>     >     >            to parser/parser.c
>     >     >          - calls to these functions, together with loops in
>     >     >     remove_installed_image_list()
>     >     >            and remove_higher_image_list() were completely
>     removed from
>     >     >     core/parser.c
>     >     >          - boolean `required` in `struct img_type` was
>     replaced with
>     >     >     `enum skip_t skip`
>     >     >          - field `img_type.skip` is assigned a value
>     detailing reason
>     >     >     for skip (if any)
>     >     >          - field `img_type.skip` is inserted into table
>     passed to Lua
>     >     >     hook (if any) end
>     >     >            retrieved after Lua call so hook function can
>     override
>     >     setting.
>     >     >          - if `img_type.skip` is non-zero the image is
>     considered
>     >     >     "skippable" and the
>     >     >            whole `img_type` structure is removed from
>     handling list.
>     >     >
>     >     >          Signed-off-by: Mauro Condarelli <mc5686@mclink.it
>     <mailto:mc5686@mclink.it>
>     >     <mailto:mc5686@mclink.it <mailto:mc5686@mclink.it>>
>     >     >     <mailto:mc5686@mclink.it <mailto:mc5686@mclink.it>
>     <mailto:mc5686@mclink.it <mailto:mc5686@mclink.it>>>>
>     >     >
>     >     >
>     >     > Next patches will:
>     >     >
>     >     >  1. make `embedded-script` Lua environment "permanent"
>     (i.e.: not
>     >     >     constrained to parsing).
>     >     >  2. add `pre-install-hook` and `post-install-hook` to artifacts.
>     >     >  3. ensure these hooks are called at the appropriate moment
>     regardless
>     >     >     of `installed-directly` setting.
>     >     >  4. add a series of standard hooks (to be defined in
>     >     `embedded-script`)
>     >     >     to be called at specific points; e.g.: startup,
>     before-parsing,
>     >     >     after-parsing, before-installation, after-installation,
>     >     >     before-bootvars, after-bootvars, on-error.
>     >     >  5. all  above hooks will be defined in `embedded-script`and
>     they will
>     >     >     share a single environment, so that variables set in one
>     hook will
>     >     >     be available to all hooks called after that.
>     >     >
>     >     > As always comments and criticism are most welcome.
>     >     >
>     >     > Regards
>     >     > Mauro
>     >     >
>     >     > --
>     >     > You received this message because you are subscribed to the
>     Google
>     >     > Groups "swupdate" group.
>     >     > To unsubscribe from this group and stop receiving emails
>     from it,
>     >     send
>     >     > an email to swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>
>     >     <mailto:swupdate%2Bunsubscribe@googlegroups.com
>     <mailto:swupdate%252Bunsubscribe@googlegroups.com>>
>     >     > <mailto:swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>
>     >     <mailto:swupdate%2Bunsubscribe@googlegroups.com
>     <mailto:swupdate%252Bunsubscribe@googlegroups.com>>>.
>     >     > To view this discussion on the web visit
>     >     >
>     >   
>      https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com
>     >
>     >     >
>     >   
>      <https://groups.google.com/d/msgid/swupdate/CABDgG8_%3D65Z1RvO9CxP7FW7k6OBBVWphJwppNTGPtqBaDnt_dg%40mail.gmail.com?utm_medium=email&utm_source=footer>.
>     >
>     >     --
>     >   
>      =====================================================================
>     >     DENX Software Engineering GmbH,      Managing Director:
>     Wolfgang Denk
>     >     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
>     Germany
>     >     Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email:
>     >     sbabic@denx.de <mailto:sbabic@denx.de> <mailto:sbabic@denx.de
>     <mailto:sbabic@denx.de>>
>     >   
>      =====================================================================
>     >
>     > --
>     > You received this message because you are subscribed to the Google
>     > Groups "swupdate" group.
>     > To unsubscribe from this group and stop receiving emails from it, send
>     > an email to swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>
>     > <mailto:swupdate+unsubscribe@googlegroups.com
>     <mailto:swupdate%2Bunsubscribe@googlegroups.com>>.
>     > To view this discussion on the web visit
>     >
>     https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com
>     >
>     <https://groups.google.com/d/msgid/swupdate/CABDgG8-FLBK0BfJVwk5i2JUNO0EPA2fXDF11k%2BL4ptpUBkdfrw%40mail.gmail.com?utm_medium=email&utm_source=footer>.
> 
> 
>     -- 
>     =====================================================================
>     DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>     HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>     Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email:
>     sbabic@denx.de <mailto:sbabic@denx.de>
>     =====================================================================
> 
> -- 
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+unsubscribe@googlegroups.com
> <mailto:swupdate+unsubscribe@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/swupdate/CABDgG89jEvRv0HS5fcHnLgBPtGXP5nYMsAzTgwWqefmSOPEvog%40mail.gmail.com
> <https://groups.google.com/d/msgid/swupdate/CABDgG89jEvRv0HS5fcHnLgBPtGXP5nYMsAzTgwWqefmSOPEvog%40mail.gmail.com?utm_medium=email&utm_source=footer>.