[PATCH] hooks: warn if reassignment overwrites previous setting

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In hook definition files, repeated assignment to Description, Exec,
Type, and When silently overwrote previous settings.  This yields a
warning now.

Signed-off-by: Stefan Klinger <[hidden email]>
---
 lib/libalpm/hook.c                           | 15 ++++++++++++++-
 test/pacman/tests/TESTS                      |  4 ++++
 test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
 test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
 6 files changed, 107 insertions(+), 1 deletion(-)
 create mode 100644 test/pacman/tests/hook-description-reused.py
 create mode 100644 test/pacman/tests/hook-exec-reused.py
 create mode 100644 test/pacman/tests/hook-type-reused.py
 create mode 100644 test/pacman/tests/hook-when-reused.py

diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
index ccde225e..51e74484 100644
--- a/lib/libalpm/hook.c
+++ b/lib/libalpm/hook.c
@@ -267,6 +267,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  struct _alpm_hook_t *hook = ctx->hook;
 
 #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
+#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__);
 
  if(!section && !key) {
  error(_("error while reading hook %s: %s\n"), file, strerror(errno));
@@ -296,6 +297,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Type") == 0) {
+                        if(t->type != 0) {
+                                warning(_("hook %s line %d: overwriting previous definition of Type\n"), file, line);
+                        }
  if(strcmp(value, "Package") == 0) {
  t->type = ALPM_HOOK_TYPE_PACKAGE;
  } else if(strcmp(value, "File") == 0) {
@@ -312,6 +316,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  } else if(strcmp(section, "Action") == 0) {
  if(strcmp(key, "When") == 0) {
+                        if(hook->when != 0) {
+                                warning(_("hook %s line %d: overwriting previous definition of When\n"), file, line);
+                        }
  if(strcmp(value, "PreTransaction") == 0) {
  hook->when = ALPM_HOOK_PRE_TRANSACTION;
  } else if(strcmp(value, "PostTransaction") == 0) {
@@ -320,6 +327,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Description") == 0) {
+                        if(hook->desc != NULL) {
+                                warning(_("hook %s line %d: overwriting previous definition of Description\n"), file, line);
+                        }
  STRDUP(hook->desc, value, return 1);
  } else if(strcmp(key, "Depends") == 0) {
  char *val;
@@ -330,7 +340,10 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  } else if(strcmp(key, "NeedsTargets") == 0) {
  hook->needs_targets = 1;
  } else if(strcmp(key, "Exec") == 0) {
- if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
+                        if(hook->cmd != NULL) {
+                                warning(_("hook %s line %d: overwriting previous definition of Exec\n"), file, line);
+                        }
+                        if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
  if(errno == EINVAL) {
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  } else {
diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
index 2d877962..4329ca90 100644
--- a/test/pacman/tests/TESTS
+++ b/test/pacman/tests/TESTS
@@ -53,6 +53,8 @@ TESTS += test/pacman/tests/fileconflict030.py
 TESTS += test/pacman/tests/fileconflict031.py
 TESTS += test/pacman/tests/fileconflict032.py
 TESTS += test/pacman/tests/hook-abortonfail.py
+TESTS += test/pacman/tests/hook-description-reused.py
+TESTS += test/pacman/tests/hook-exec-reused.py
 TESTS += test/pacman/tests/hook-exec-with-arguments.py
 TESTS += test/pacman/tests/hook-file-change-packages.py
 TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
@@ -63,7 +65,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
 TESTS += test/pacman/tests/hook-target-list.py
+TESTS += test/pacman/tests/hook-type-reused.py
 TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
+TESTS += test/pacman/tests/hook-when-reused.py
 TESTS += test/pacman/tests/ignore001.py
 TESTS += test/pacman/tests/ignore002.py
 TESTS += test/pacman/tests/ignore003.py
diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
new file mode 100644
index 00000000..5826a30e
--- /dev/null
+++ b/test/pacman/tests/hook-description-reused.py
@@ -0,0 +1,23 @@
+self.description = "Hook using multiple Description's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        Description = lala
+        Description = foobar
+        When = PreTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
new file mode 100644
index 00000000..6fe53151
--- /dev/null
+++ b/test/pacman/tests/hook-exec-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple Exec's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
new file mode 100644
index 00000000..8c4dfd0f
--- /dev/null
+++ b/test/pacman/tests/hook-type-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple Type's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Type = File
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
new file mode 100644
index 00000000..07c3bc5e
--- /dev/null
+++ b/test/pacman/tests/hook-when-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple When's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PreTransaction
+        Exec = /bin/date
+        When = PostTransaction
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
--
2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Silvan Jegen
Hi

On Mon, Jan 2, 2017 at 4:19 PM, Stefan Klinger <[hidden email]> wrote:

> In hook definition files, repeated assignment to Description, Exec,
> Type, and When silently overwrote previous settings.  This yields a
> warning now.
>
> Signed-off-by: Stefan Klinger <[hidden email]>
> ---
>  lib/libalpm/hook.c                           | 15 ++++++++++++++-
>  test/pacman/tests/TESTS                      |  4 ++++
>  test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
>  test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
>  6 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 test/pacman/tests/hook-description-reused.py
>  create mode 100644 test/pacman/tests/hook-exec-reused.py
>  create mode 100644 test/pacman/tests/hook-type-reused.py
>  create mode 100644 test/pacman/tests/hook-when-reused.py

Looks good to me but I have not tested it.

One nitpick below.


> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
> index ccde225e..51e74484 100644
> --- a/lib/libalpm/hook.c
> +++ b/lib/libalpm/hook.c
> [...]
>  TESTS += test/pacman/tests/ignore003.py
> diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
> new file mode 100644
> index 00000000..5826a30e
> --- /dev/null
> +++ b/test/pacman/tests/hook-description-reused.py
> @@ -0,0 +1,23 @@
> +self.description = "Hook using multiple Description's"

The 's looks like a possessive 's to me (instead of a plural). Writing

+self.description = "Hook using multiple 'Description's"

reads better to me (the same is true for all other self.descriptions
further down of course).

Hey, I gave you a fair nitpick-warning! :P


Cheers,

Silvan

> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        Description = lala
> +        Description = foobar
> +        When = PreTransaction
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
> diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
> new file mode 100644
> index 00000000..6fe53151
> --- /dev/null
> +++ b/test/pacman/tests/hook-exec-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple Exec's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PostTransaction
> +        Exec = /bin/date
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
> diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
> new file mode 100644
> index 00000000..8c4dfd0f
> --- /dev/null
> +++ b/test/pacman/tests/hook-type-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple Type's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Type = File
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PostTransaction
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
> diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
> new file mode 100644
> index 00000000..07c3bc5e
> --- /dev/null
> +++ b/test/pacman/tests/hook-when-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple When's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PreTransaction
> +        Exec = /bin/date
> +        When = PostTransaction
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
> --
> 2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Eli Schwartz
In reply to this post by Stefan Klinger
On 01/02/2017 10:19 AM, Stefan Klinger wrote:
> +                        if(t->type != 0) {
> +                                warning(_("hook %s line %d: overwriting previous definition of Type\n"), file, line);
> +                        }
...
> +                        if(hook->when != 0) {
> +                                warning(_("hook %s line %d: overwriting previous definition of When\n"), file, line);
> +                        }
...
> +                        if(hook->desc != NULL) {
> +                                warning(_("hook %s line %d: overwriting previous definition of Description\n"), file, line);
> +                        }
...
> +                        if(hook->cmd != NULL) {
> +                                warning(_("hook %s line %d: overwriting previous definition of Exec\n"), file, line);
> +                        }

Look at all the times error is used. ;) You should be formatting the
warning with the definition type as well.

This allows a single translation to be used for all four.

--
Eli Schwartz


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Andrew Gregory
In reply to this post by Stefan Klinger
On 01/02/17 at 04:19pm, Stefan Klinger wrote:

> In hook definition files, repeated assignment to Description, Exec,
> Type, and When silently overwrote previous settings.  This yields a
> warning now.
>
> Signed-off-by: Stefan Klinger <[hidden email]>
> ---
>  lib/libalpm/hook.c                           | 15 ++++++++++++++-
>  test/pacman/tests/TESTS                      |  4 ++++
>  test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
>  test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
>  6 files changed, 107 insertions(+), 1 deletion(-)
>  create mode 100644 test/pacman/tests/hook-description-reused.py
>  create mode 100644 test/pacman/tests/hook-exec-reused.py
>  create mode 100644 test/pacman/tests/hook-type-reused.py
>  create mode 100644 test/pacman/tests/hook-when-reused.py
>
> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
> index ccde225e..51e74484 100644
> --- a/lib/libalpm/hook.c
> +++ b/lib/libalpm/hook.c
> @@ -267,6 +267,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   struct _alpm_hook_t *hook = ctx->hook;
>  
>  #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
> +#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__);
>  
>   if(!section && !key) {
>   error(_("error while reading hook %s: %s\n"), file, strerror(errno));
> @@ -296,6 +297,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
>   }
>   } else if(strcmp(key, "Type") == 0) {
> +                        if(t->type != 0) {
> +                                warning(_("hook %s line %d: overwriting previous definition of Type\n"), file, line);
> +                        }

We use tabs for indenting C files, not spaces.
Reply | Threaded
Open this post in threaded view
|

[PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In hook definition files, repeated assignment to Description, Exec,
Type, and When silently overwrote previous settings.  This yields a
warning now.

Signed-off-by: Stefan Klinger <[hidden email]>
---
 lib/libalpm/hook.c                           | 16 +++++++++++++++-
 test/pacman/tests/TESTS                      |  4 ++++
 test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
 test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
 6 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 test/pacman/tests/hook-description-reused.py
 create mode 100644 test/pacman/tests/hook-exec-reused.py
 create mode 100644 test/pacman/tests/hook-type-reused.py
 create mode 100644 test/pacman/tests/hook-when-reused.py

diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
index ccde225e..4b3fa3cb 100644
--- a/lib/libalpm/hook.c
+++ b/lib/libalpm/hook.c
@@ -267,6 +267,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  struct _alpm_hook_t *hook = ctx->hook;
 
 #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
+#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__);
 
  if(!section && !key) {
  error(_("error while reading hook %s: %s\n"), file, strerror(errno));
@@ -296,6 +297,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Type") == 0) {
+ if(t->type != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Type");
+ }
  if(strcmp(value, "Package") == 0) {
  t->type = ALPM_HOOK_TYPE_PACKAGE;
  } else if(strcmp(value, "File") == 0) {
@@ -312,6 +316,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  } else if(strcmp(section, "Action") == 0) {
  if(strcmp(key, "When") == 0) {
+ if(hook->when != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "When");
+ }
  if(strcmp(value, "PreTransaction") == 0) {
  hook->when = ALPM_HOOK_PRE_TRANSACTION;
  } else if(strcmp(value, "PostTransaction") == 0) {
@@ -320,6 +327,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Description") == 0) {
+ if(hook->desc != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
+ }
  STRDUP(hook->desc, value, return 1);
  } else if(strcmp(key, "Depends") == 0) {
  char *val;
@@ -330,12 +340,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  } else if(strcmp(key, "NeedsTargets") == 0) {
  hook->needs_targets = 1;
  } else if(strcmp(key, "Exec") == 0) {
+ if(hook->cmd != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
+ }
  if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
  if(errno == EINVAL) {
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  } else {
  error(_("hook %s line %d: unable to set option (%s)\n"),
- file, line, strerror(errno));
+      file, line, strerror(errno));
  }
  }
  } else {
@@ -344,6 +357,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
 
 #undef error
+#undef warning
 
  return 0;
 }
diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
index 2d877962..4329ca90 100644
--- a/test/pacman/tests/TESTS
+++ b/test/pacman/tests/TESTS
@@ -53,6 +53,8 @@ TESTS += test/pacman/tests/fileconflict030.py
 TESTS += test/pacman/tests/fileconflict031.py
 TESTS += test/pacman/tests/fileconflict032.py
 TESTS += test/pacman/tests/hook-abortonfail.py
+TESTS += test/pacman/tests/hook-description-reused.py
+TESTS += test/pacman/tests/hook-exec-reused.py
 TESTS += test/pacman/tests/hook-exec-with-arguments.py
 TESTS += test/pacman/tests/hook-file-change-packages.py
 TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
@@ -63,7 +65,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
 TESTS += test/pacman/tests/hook-target-list.py
+TESTS += test/pacman/tests/hook-type-reused.py
 TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
+TESTS += test/pacman/tests/hook-when-reused.py
 TESTS += test/pacman/tests/ignore001.py
 TESTS += test/pacman/tests/ignore002.py
 TESTS += test/pacman/tests/ignore003.py
diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
new file mode 100644
index 00000000..87637e80
--- /dev/null
+++ b/test/pacman/tests/hook-description-reused.py
@@ -0,0 +1,23 @@
+self.description = "Hook using multiple 'Description's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        Description = lala
+        Description = foobar
+        When = PreTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
new file mode 100644
index 00000000..38423177
--- /dev/null
+++ b/test/pacman/tests/hook-exec-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Exec's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
new file mode 100644
index 00000000..472c8caf
--- /dev/null
+++ b/test/pacman/tests/hook-type-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Type's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Type = File
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
new file mode 100644
index 00000000..85a93db8
--- /dev/null
+++ b/test/pacman/tests/hook-when-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'When's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PreTransaction
+        Exec = /bin/date
+        When = PostTransaction
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
--
2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In reply to this post by Eli Schwartz
On 2017-Jan-03, Eli Schwartz wrote with possible deletions:
> On 01/02/2017 10:19 AM, Stefan Klinger wrote:
> > +        warning(_("hook %s line %d: overwriting previous definition of Type\n"), file, line);
> > +        warning(_("hook %s line %d: overwriting previous definition of When\n"), file, line);
> > +        warning(_("hook %s line %d: overwriting previous definition of Description\n"), file, line);
> > +        warning(_("hook %s line %d: overwriting previous definition of Exec\n"), file, line);
>
> Look at all the times error is used. ;) You should be formatting the
> warning with the definition type as well.

Yeah, I had that before but was unsure whether it's appreciated to do
formatting with a constant value at runtime.  But I'll change that.


Also, I'd like to change

    #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;

to

    #define error(...) do { _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1; } while (0)

see [1].  Also move `error` and `warning` both to the top and use
`error` everywhere where appropriate.

____________________
[1] http://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for#answer-257425



--
http://stefan-klinger.de                                      o/X
Send plain text messages only, not exceeding 32kB.            /\/
                                                                \
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In reply to this post by Silvan Jegen
On 2017-Jan-03, Silvan Jegen wrote with possible deletions:

> On Mon, Jan 2, 2017 at 4:19 PM, Stefan Klinger <[hidden email]> wrote:
> > In hook definition files, repeated assignment to Description, Exec,
> > Type, and When silently overwrote previous settings.  This yields a
> > warning now.
>
> Looks good to me but I have not tested it.
>
> One nitpick below.
>
> > +self.description = "Hook using multiple Description's"
>
> The 's looks like a possessive 's to me (instead of a plural). Writing
>
> +self.description = "Hook using multiple 'Description's"

That's how it was intended.  I'll fix it.


--
http://stefan-klinger.de                                      o/X
Send plain text messages only, not exceeding 32kB.            /\/
                                                                \
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In reply to this post by Andrew Gregory
On 2017-Jan-03, Andrew Gregory wrote with possible deletions:
> We use tabs for indenting C files, not spaces.

Ok, didn't think of that.

Do you have a style-checking script I could use?

Do you prefer a patch with all commits squashed into one, or rather
many small commits (that you could squash yourself)?


--
http://stefan-klinger.de                                      o/X
Send plain text messages only, not exceeding 32kB.            /\/
                                                                \
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Allan McRae
On 04/01/17 02:01, [hidden email] wrote:
> Do you prefer a patch with all commits squashed into one, or rather
> many small commits (that you could squash yourself)?

Just one squashed patch thanks.

A
Reply | Threaded
Open this post in threaded view
|

[PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
In hook definition files, repeated assignment to Description, Exec,
Type, and When silently overwrote previous settings.  This yields a
warning now.

Signed-off-by: Stefan Klinger <[hidden email]>
---
 lib/libalpm/hook.c                           | 16 +++++++++++++++-
 test/pacman/tests/TESTS                      |  4 ++++
 test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
 test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
 6 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 test/pacman/tests/hook-description-reused.py
 create mode 100644 test/pacman/tests/hook-exec-reused.py
 create mode 100644 test/pacman/tests/hook-type-reused.py
 create mode 100644 test/pacman/tests/hook-when-reused.py

diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
index 4ec2a906..dd20a55c 100644
--- a/lib/libalpm/hook.c
+++ b/lib/libalpm/hook.c
@@ -267,6 +267,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  struct _alpm_hook_t *hook = ctx->hook;
 
 #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
+#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__);
 
  if(!section && !key) {
  error(_("error while reading hook %s: %s\n"), file, strerror(errno));
@@ -296,6 +297,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Type") == 0) {
+ if(t->type != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Type");
+ }
  if(strcmp(value, "Package") == 0) {
  t->type = ALPM_HOOK_TYPE_PACKAGE;
  } else if(strcmp(value, "File") == 0) {
@@ -312,6 +316,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  } else if(strcmp(section, "Action") == 0) {
  if(strcmp(key, "When") == 0) {
+ if(hook->when != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "When");
+ }
  if(strcmp(value, "PreTransaction") == 0) {
  hook->when = ALPM_HOOK_PRE_TRANSACTION;
  } else if(strcmp(value, "PostTransaction") == 0) {
@@ -320,6 +327,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Description") == 0) {
+ if(hook->desc != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
+ }
  STRDUP(hook->desc, value, return 1);
  } else if(strcmp(key, "Depends") == 0) {
  char *val;
@@ -330,12 +340,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  } else if(strcmp(key, "NeedsTargets") == 0) {
  hook->needs_targets = 1;
  } else if(strcmp(key, "Exec") == 0) {
+ if(hook->cmd != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
+ }
  if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
  if(errno == EINVAL) {
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  } else {
  error(_("hook %s line %d: unable to set option (%s)\n"),
- file, line, strerror(errno));
+      file, line, strerror(errno));
  }
  }
  } else {
@@ -344,6 +357,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
 
 #undef error
+#undef warning
 
  return 0;
 }
diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
index 11d1c38c..dd191175 100644
--- a/test/pacman/tests/TESTS
+++ b/test/pacman/tests/TESTS
@@ -54,6 +54,8 @@ TESTS += test/pacman/tests/fileconflict030.py
 TESTS += test/pacman/tests/fileconflict031.py
 TESTS += test/pacman/tests/fileconflict032.py
 TESTS += test/pacman/tests/hook-abortonfail.py
+TESTS += test/pacman/tests/hook-description-reused.py
+TESTS += test/pacman/tests/hook-exec-reused.py
 TESTS += test/pacman/tests/hook-exec-with-arguments.py
 TESTS += test/pacman/tests/hook-file-change-packages.py
 TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
@@ -64,7 +66,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
 TESTS += test/pacman/tests/hook-target-list.py
+TESTS += test/pacman/tests/hook-type-reused.py
 TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
+TESTS += test/pacman/tests/hook-when-reused.py
 TESTS += test/pacman/tests/ignore001.py
 TESTS += test/pacman/tests/ignore002.py
 TESTS += test/pacman/tests/ignore003.py
diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
new file mode 100644
index 00000000..87637e80
--- /dev/null
+++ b/test/pacman/tests/hook-description-reused.py
@@ -0,0 +1,23 @@
+self.description = "Hook using multiple 'Description's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        Description = lala
+        Description = foobar
+        When = PreTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
new file mode 100644
index 00000000..38423177
--- /dev/null
+++ b/test/pacman/tests/hook-exec-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Exec's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
new file mode 100644
index 00000000..472c8caf
--- /dev/null
+++ b/test/pacman/tests/hook-type-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Type's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Type = File
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
new file mode 100644
index 00000000..85a93db8
--- /dev/null
+++ b/test/pacman/tests/hook-when-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'When's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PreTransaction
+        Exec = /bin/date
+        When = PostTransaction
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
--
2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Allan McRae
On 03/01/17 01:19, Stefan Klinger wrote:

> In hook definition files, repeated assignment to Description, Exec,
> Type, and When silently overwrote previous settings.  This yields a
> warning now.
>
> Signed-off-by: Stefan Klinger <[hidden email]>
> ---
>  lib/libalpm/hook.c                           | 16 +++++++++++++++-
>  test/pacman/tests/TESTS                      |  4 ++++
>  test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
>  test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
>  test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
>  6 files changed, 108 insertions(+), 1 deletion(-)
>  create mode 100644 test/pacman/tests/hook-description-reused.py
>  create mode 100644 test/pacman/tests/hook-exec-reused.py
>  create mode 100644 test/pacman/tests/hook-type-reused.py
>  create mode 100644 test/pacman/tests/hook-when-reused.py
>

Looks good.

A
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Allan McRae
In reply to this post by Stefan Klinger
On 03/01/17 01:19, Stefan Klinger wrote:
> In hook definition files, repeated assignment to Description, Exec,
> Type, and When silently overwrote previous settings.  This yields a
> warning now.
>
> Signed-off-by: Stefan Klinger <[hidden email]>
> ---

On closer review.... y ou have exposed two memory leaks here.  Please
fix and resubmit.

Thanks,
Allan

<snip>

>   } else if(strcmp(key, "Description") == 0) {
> + if(hook->desc != NULL) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
> + }

FREE(hook->desc);

>   STRDUP(hook->desc, value, return 1);
>   } else if(strcmp(key, "Depends") == 0) {
>   char *val;
> @@ -330,12 +340,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   } else if(strcmp(key, "NeedsTargets") == 0) {
>   hook->needs_targets = 1;
>   } else if(strcmp(key, "Exec") == 0) {
> + if(hook->cmd != NULL) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
> + }

_alpm_wordsplit_free(hook->cmd);

>   if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
>   if(errno == EINVAL) {
>   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
>   } else {
>   error(_("hook %s line %d: unable to set option (%s)\n"),
> - file, line, strerror(errno));
> +      file, line, strerror(errno));
>   }
>   }
>   } else {
Reply | Threaded
Open this post in threaded view
|

[PATCH] hooks: warn if reassignment overwrites previous setting

Stefan Klinger
hooks: complain if multiple Exec options in alpm hook

hooks: test for reassign Exec

hook: Make reassignment to Exec yield a warning

hooks: more consistency checks in hook definition

hooks: fixed typo in test descriptions

hooks: refactored warnings into one common message

hooks: using tabs for indentation

fixed 2 space leaks when overwriting hook definitions

hook: better macro definitions, more consistent usage of them
Signed-off-by: Stefan Klinger <[hidden email]>
---
 lib/libalpm/hook.c                           | 58 ++++++++++++++--------------
 test/pacman/tests/TESTS                      |  4 ++
 test/pacman/tests/hook-description-reused.py | 23 +++++++++++
 test/pacman/tests/hook-exec-reused.py        | 22 +++++++++++
 test/pacman/tests/hook-type-reused.py        | 22 +++++++++++
 test/pacman/tests/hook-when-reused.py        | 22 +++++++++++
 6 files changed, 123 insertions(+), 28 deletions(-)
 create mode 100644 test/pacman/tests/hook-description-reused.py
 create mode 100644 test/pacman/tests/hook-exec-reused.py
 create mode 100644 test/pacman/tests/hook-type-reused.py
 create mode 100644 test/pacman/tests/hook-when-reused.py

diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
index 4ec2a906..fdb6d475 100644
--- a/lib/libalpm/hook.c
+++ b/lib/libalpm/hook.c
@@ -30,6 +30,10 @@
 #include "trans.h"
 #include "util.h"
 
+
+#define error(...) do { _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1; } while (0)
+#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__)
+
 enum _alpm_hook_op_t {
  ALPM_HOOK_OP_INSTALL = (1 << 0),
  ALPM_HOOK_OP_UPGRADE = (1 << 1),
@@ -103,20 +107,17 @@ static int _alpm_trigger_validate(alpm_handle_t *handle,
 
  if(trigger->targets == NULL) {
  ret = -1;
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("Missing trigger targets in hook: %s\n"), file);
+ error(_("Missing trigger targets in hook: %s\n"), file);
  }
 
  if(trigger->type == 0) {
  ret = -1;
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("Missing trigger type in hook: %s\n"), file);
+ error(_("Missing trigger type in hook: %s\n"), file);
  }
 
  if(trigger->op == 0) {
  ret = -1;
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("Missing trigger operation in hook: %s\n"), file);
+ error(_("Missing trigger operation in hook: %s\n"), file);
  }
 
  return ret;
@@ -142,14 +143,12 @@ static int _alpm_hook_validate(alpm_handle_t *handle,
 
  if(hook->cmd == NULL) {
  ret = -1;
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("Missing Exec option in hook: %s\n"), file);
+ error(_("Missing Exec option in hook: %s\n"), file);
  }
 
  if(hook->when == 0) {
  ret = -1;
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("Missing When option in hook: %s\n"), file);
+ error(_("Missing When option in hook: %s\n"), file);
  } else if(hook->when != ALPM_HOOK_PRE_TRANSACTION && hook->abort_on_fail) {
  _alpm_log(handle, ALPM_LOG_WARNING,
  _("AbortOnFail set for PostTransaction hook: %s\n"), file);
@@ -266,8 +265,6 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  alpm_handle_t *handle = ctx->handle;
  struct _alpm_hook_t *hook = ctx->hook;
 
-#define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
-
  if(!section && !key) {
  error(_("error while reading hook %s: %s\n"), file, strerror(errno));
  } else if(!section) {
@@ -296,6 +293,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Type") == 0) {
+ if(t->type != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Type");
+ }
  if(strcmp(value, "Package") == 0) {
  t->type = ALPM_HOOK_TYPE_PACKAGE;
  } else if(strcmp(value, "File") == 0) {
@@ -312,6 +312,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  } else if(strcmp(section, "Action") == 0) {
  if(strcmp(key, "When") == 0) {
+ if(hook->when != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "When");
+ }
  if(strcmp(value, "PreTransaction") == 0) {
  hook->when = ALPM_HOOK_PRE_TRANSACTION;
  } else if(strcmp(value, "PostTransaction") == 0) {
@@ -320,6 +323,10 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Description") == 0) {
+ if(hook->desc != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
+ FREE(hook->desc);
+ }
  STRDUP(hook->desc, value, return 1);
  } else if(strcmp(key, "Depends") == 0) {
  char *val;
@@ -330,12 +337,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  } else if(strcmp(key, "NeedsTargets") == 0) {
  hook->needs_targets = 1;
  } else if(strcmp(key, "Exec") == 0) {
+ if(hook->cmd != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
+ _alpm_wordsplit_free(hook->cmd);
+ }
  if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
  if(errno == EINVAL) {
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  } else {
- error(_("hook %s line %d: unable to set option (%s)\n"),
- file, line, strerror(errno));
+ error(_("hook %s line %d: unable to set option (%s)\n"), file, line, strerror(errno));
  }
  }
  } else {
@@ -343,8 +353,6 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  }
 
-#undef error
-
  return 0;
 }
 
@@ -594,8 +602,7 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook)
 
  for(i = hook->depends; i; i = i->next) {
  if(!alpm_find_satisfier(pkgs, i->data)) {
- _alpm_log(handle, ALPM_LOG_ERROR, _("unable to run hook %s: %s\n"),
- hook->name, _("could not satisfy dependencies"));
+ error(_("unable to run hook %s: %s\n"), hook->name, _("could not satisfy dependencies"));
  return -1;
  }
  }
@@ -629,8 +636,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
  DIR *d;
 
  if((dirlen = strlen(i->data)) >= PATH_MAX) {
- _alpm_log(handle, ALPM_LOG_ERROR, _("could not open directory: %s: %s\n"),
- (char *)i->data, strerror(ENAMETOOLONG));
+ error(_("could not open directory: %s: %s\n"), (char *)i->data, strerror(ENAMETOOLONG));
  ret = -1;
  continue;
  }
@@ -640,8 +646,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
  if(errno == ENOENT) {
  continue;
  } else {
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("could not open directory: %s: %s\n"), path, strerror(errno));
+ error(_("could not open directory: %s: %s\n"), path, strerror(errno));
  ret = -1;
  continue;
  }
@@ -657,8 +662,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
  }
 
  if((name_len = strlen(entry->d_name)) >= PATH_MAX - dirlen) {
- _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file: %s%s: %s\n"),
- path, entry->d_name, strerror(ENAMETOOLONG));
+ error(_("could not open file: %s%s: %s\n"), path, entry->d_name, strerror(ENAMETOOLONG));
  ret = -1;
  continue;
  }
@@ -676,8 +680,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
  }
 
  if(stat(path, &buf) != 0) {
- _alpm_log(handle, ALPM_LOG_ERROR,
- _("could not stat file %s: %s\n"), path, strerror(errno));
+ error(_("could not stat file %s: %s\n"), path, strerror(errno));
  ret = -1;
  continue;
  }
@@ -703,8 +706,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
  hooks = alpm_list_add(hooks, ctx.hook);
  }
  if(errno != 0) {
- _alpm_log(handle, ALPM_LOG_ERROR, _("could not read directory: %s: %s\n"),
- (char *) i->data, strerror(errno));
+ error(_("could not read directory: %s: %s\n"), (char *) i->data, strerror(errno));
  ret = -1;
  }
 
diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
index 11d1c38c..dd191175 100644
--- a/test/pacman/tests/TESTS
+++ b/test/pacman/tests/TESTS
@@ -54,6 +54,8 @@ TESTS += test/pacman/tests/fileconflict030.py
 TESTS += test/pacman/tests/fileconflict031.py
 TESTS += test/pacman/tests/fileconflict032.py
 TESTS += test/pacman/tests/hook-abortonfail.py
+TESTS += test/pacman/tests/hook-description-reused.py
+TESTS += test/pacman/tests/hook-exec-reused.py
 TESTS += test/pacman/tests/hook-exec-with-arguments.py
 TESTS += test/pacman/tests/hook-file-change-packages.py
 TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
@@ -64,7 +66,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
 TESTS += test/pacman/tests/hook-target-list.py
+TESTS += test/pacman/tests/hook-type-reused.py
 TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
+TESTS += test/pacman/tests/hook-when-reused.py
 TESTS += test/pacman/tests/ignore001.py
 TESTS += test/pacman/tests/ignore002.py
 TESTS += test/pacman/tests/ignore003.py
diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
new file mode 100644
index 00000000..87637e80
--- /dev/null
+++ b/test/pacman/tests/hook-description-reused.py
@@ -0,0 +1,23 @@
+self.description = "Hook using multiple 'Description's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        Description = lala
+        Description = foobar
+        When = PreTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
new file mode 100644
index 00000000..38423177
--- /dev/null
+++ b/test/pacman/tests/hook-exec-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Exec's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
new file mode 100644
index 00000000..472c8caf
--- /dev/null
+++ b/test/pacman/tests/hook-type-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Type's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Type = File
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
new file mode 100644
index 00000000..85a93db8
--- /dev/null
+++ b/test/pacman/tests/hook-when-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'When's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PreTransaction
+        Exec = /bin/date
+        When = PostTransaction
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
--
2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Andrew Gregory
In reply to this post by Allan McRae
On 01/12/17 at 09:39pm, Allan McRae wrote:

> On 03/01/17 01:19, Stefan Klinger wrote:
> > In hook definition files, repeated assignment to Description, Exec,
> > Type, and When silently overwrote previous settings.  This yields a
> > warning now.
> >
> > Signed-off-by: Stefan Klinger <[hidden email]>
> > ---
>
> On closer review.... y ou have exposed two memory leaks here.  Please
> fix and resubmit.
>
> Thanks,
> Allan
>
> <snip>
>
> >   } else if(strcmp(key, "Description") == 0) {
> > + if(hook->desc != NULL) {
> > + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
> > + }
>
> FREE(hook->desc);
>
> >   STRDUP(hook->desc, value, return 1);
> >   } else if(strcmp(key, "Depends") == 0) {
> >   char *val;
> > @@ -330,12 +340,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
> >   } else if(strcmp(key, "NeedsTargets") == 0) {
> >   hook->needs_targets = 1;
> >   } else if(strcmp(key, "Exec") == 0) {
> > + if(hook->cmd != NULL) {
> > + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
> > + }
>
> _alpm_wordsplit_free(hook->cmd);
>
> >   if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
> >   if(errno == EINVAL) {
> >   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
> >   } else {
> >   error(_("hook %s line %d: unable to set option (%s)\n"),
> > - file, line, strerror(errno));
> > +      file, line, strerror(errno));

While you're at it, undo this change.

> >   }
> >   }
> >   } else {
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Andrew Gregory
In reply to this post by Stefan Klinger
On 01/02/17 at 04:19pm, Stefan Klinger wrote:

> hooks: complain if multiple Exec options in alpm hook
>
> hooks: test for reassign Exec
>
> hook: Make reassignment to Exec yield a warning
>
> hooks: more consistency checks in hook definition
>
> hooks: fixed typo in test descriptions
>
> hooks: refactored warnings into one common message
>
> hooks: using tabs for indentation
>
> fixed 2 space leaks when overwriting hook definitions
>
> hook: better macro definitions, more consistent usage of them
> Signed-off-by: Stefan Klinger <[hidden email]>
> ---
>  lib/libalpm/hook.c                           | 58 ++++++++++++++--------------
>  test/pacman/tests/TESTS                      |  4 ++
>  test/pacman/tests/hook-description-reused.py | 23 +++++++++++
>  test/pacman/tests/hook-exec-reused.py        | 22 +++++++++++
>  test/pacman/tests/hook-type-reused.py        | 22 +++++++++++
>  test/pacman/tests/hook-when-reused.py        | 22 +++++++++++
>  6 files changed, 123 insertions(+), 28 deletions(-)
>  create mode 100644 test/pacman/tests/hook-description-reused.py
>  create mode 100644 test/pacman/tests/hook-exec-reused.py
>  create mode 100644 test/pacman/tests/hook-type-reused.py
>  create mode 100644 test/pacman/tests/hook-when-reused.py
>
> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
> index 4ec2a906..fdb6d475 100644
> --- a/lib/libalpm/hook.c
> +++ b/lib/libalpm/hook.c
> @@ -30,6 +30,10 @@
>  #include "trans.h"
>  #include "util.h"
>  
> +
> +#define error(...) do { _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1; } while (0)
> +#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__)

The error macro was written specifically for the section of code where
it was used.  Using it elsewhere causes those functions to return
early.  They were specifically written not to so that we could present
all relevant errors to the user at once.

>  enum _alpm_hook_op_t {
>   ALPM_HOOK_OP_INSTALL = (1 << 0),
>   ALPM_HOOK_OP_UPGRADE = (1 << 1),
> @@ -103,20 +107,17 @@ static int _alpm_trigger_validate(alpm_handle_t *handle,
>  
>   if(trigger->targets == NULL) {
>   ret = -1;
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("Missing trigger targets in hook: %s\n"), file);
> + error(_("Missing trigger targets in hook: %s\n"), file);

NACK

>   }
>  
>   if(trigger->type == 0) {
>   ret = -1;
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("Missing trigger type in hook: %s\n"), file);
> + error(_("Missing trigger type in hook: %s\n"), file);

NACK

>   }
>  
>   if(trigger->op == 0) {
>   ret = -1;
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("Missing trigger operation in hook: %s\n"), file);
> + error(_("Missing trigger operation in hook: %s\n"), file);

NACK

>   }
>  
>   return ret;
> @@ -142,14 +143,12 @@ static int _alpm_hook_validate(alpm_handle_t *handle,
>  
>   if(hook->cmd == NULL) {
>   ret = -1;
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("Missing Exec option in hook: %s\n"), file);
> + error(_("Missing Exec option in hook: %s\n"), file);

NACK

>   }
>  
>   if(hook->when == 0) {
>   ret = -1;
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("Missing When option in hook: %s\n"), file);
> + error(_("Missing When option in hook: %s\n"), file);

NACK

>   } else if(hook->when != ALPM_HOOK_PRE_TRANSACTION && hook->abort_on_fail) {
>   _alpm_log(handle, ALPM_LOG_WARNING,
>   _("AbortOnFail set for PostTransaction hook: %s\n"), file);
> @@ -266,8 +265,6 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   alpm_handle_t *handle = ctx->handle;
>   struct _alpm_hook_t *hook = ctx->hook;
>  
> -#define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
> -
>   if(!section && !key) {
>   error(_("error while reading hook %s: %s\n"), file, strerror(errno));
>   } else if(!section) {
> @@ -296,6 +293,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
>   }
>   } else if(strcmp(key, "Type") == 0) {
> + if(t->type != 0) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Type");

Please try to keep code wrapped close to 80 columns.

> + }
>   if(strcmp(value, "Package") == 0) {
>   t->type = ALPM_HOOK_TYPE_PACKAGE;
>   } else if(strcmp(value, "File") == 0) {
> @@ -312,6 +312,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   }
>   } else if(strcmp(section, "Action") == 0) {
>   if(strcmp(key, "When") == 0) {
> + if(hook->when != 0) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "When");

Ditto.

> + }
>   if(strcmp(value, "PreTransaction") == 0) {
>   hook->when = ALPM_HOOK_PRE_TRANSACTION;
>   } else if(strcmp(value, "PostTransaction") == 0) {
> @@ -320,6 +323,10 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
>   }
>   } else if(strcmp(key, "Description") == 0) {
> + if(hook->desc != NULL) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");

Ditto.

> + FREE(hook->desc);
> + }
>   STRDUP(hook->desc, value, return 1);
>   } else if(strcmp(key, "Depends") == 0) {
>   char *val;
> @@ -330,12 +337,15 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   } else if(strcmp(key, "NeedsTargets") == 0) {
>   hook->needs_targets = 1;
>   } else if(strcmp(key, "Exec") == 0) {
> + if(hook->cmd != NULL) {
> + warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");

Ditto.

> + _alpm_wordsplit_free(hook->cmd);
> + }
>   if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
>   if(errno == EINVAL) {
>   error(_("hook %s line %d: invalid value %s\n"), file, line, value);
>   } else {
> - error(_("hook %s line %d: unable to set option (%s)\n"),
> - file, line, strerror(errno));
> + error(_("hook %s line %d: unable to set option (%s)\n"), file, line, strerror(errno));

Ditto.

>   }
>   }
>   } else {
> @@ -343,8 +353,6 @@ static int _alpm_hook_parse_cb(const char *file, int line,
>   }
>   }
>  
> -#undef error
> -
>   return 0;
>  }
>  
> @@ -594,8 +602,7 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook)
>  
>   for(i = hook->depends; i; i = i->next) {
>   if(!alpm_find_satisfier(pkgs, i->data)) {
> - _alpm_log(handle, ALPM_LOG_ERROR, _("unable to run hook %s: %s\n"),
> - hook->name, _("could not satisfy dependencies"));
> + error(_("unable to run hook %s: %s\n"), hook->name, _("could not satisfy dependencies"));

This is the only place the error macro could actually be used, and you
left the return statement.

>   return -1;
>   }
>   }
> @@ -629,8 +636,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
>   DIR *d;
>  
>   if((dirlen = strlen(i->data)) >= PATH_MAX) {
> - _alpm_log(handle, ALPM_LOG_ERROR, _("could not open directory: %s: %s\n"),
> - (char *)i->data, strerror(ENAMETOOLONG));
> + error(_("could not open directory: %s: %s\n"), (char *)i->data, strerror(ENAMETOOLONG));

NACK

>   ret = -1;
>   continue;
>   }
> @@ -640,8 +646,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
>   if(errno == ENOENT) {
>   continue;
>   } else {
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("could not open directory: %s: %s\n"), path, strerror(errno));
> + error(_("could not open directory: %s: %s\n"), path, strerror(errno));

NACK

>   ret = -1;
>   continue;
>   }
> @@ -657,8 +662,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
>   }
>  
>   if((name_len = strlen(entry->d_name)) >= PATH_MAX - dirlen) {
> - _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file: %s%s: %s\n"),
> - path, entry->d_name, strerror(ENAMETOOLONG));
> + error(_("could not open file: %s%s: %s\n"), path, entry->d_name, strerror(ENAMETOOLONG));

NACK

>   ret = -1;
>   continue;
>   }
> @@ -676,8 +680,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
>   }
>  
>   if(stat(path, &buf) != 0) {
> - _alpm_log(handle, ALPM_LOG_ERROR,
> - _("could not stat file %s: %s\n"), path, strerror(errno));
> + error(_("could not stat file %s: %s\n"), path, strerror(errno));

NACK

>   ret = -1;
>   continue;
>   }
> @@ -703,8 +706,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when)
>   hooks = alpm_list_add(hooks, ctx.hook);
>   }
>   if(errno != 0) {
> - _alpm_log(handle, ALPM_LOG_ERROR, _("could not read directory: %s: %s\n"),
> - (char *) i->data, strerror(errno));
> + error(_("could not read directory: %s: %s\n"), (char *) i->data, strerror(errno));

NACK

>   ret = -1;
>   }
>  
> diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
> index 11d1c38c..dd191175 100644
> --- a/test/pacman/tests/TESTS
> +++ b/test/pacman/tests/TESTS
> @@ -54,6 +54,8 @@ TESTS += test/pacman/tests/fileconflict030.py
>  TESTS += test/pacman/tests/fileconflict031.py
>  TESTS += test/pacman/tests/fileconflict032.py
>  TESTS += test/pacman/tests/hook-abortonfail.py
> +TESTS += test/pacman/tests/hook-description-reused.py
> +TESTS += test/pacman/tests/hook-exec-reused.py
>  TESTS += test/pacman/tests/hook-exec-with-arguments.py
>  TESTS += test/pacman/tests/hook-file-change-packages.py
>  TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
> @@ -64,7 +66,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
>  TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
>  TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
>  TESTS += test/pacman/tests/hook-target-list.py
> +TESTS += test/pacman/tests/hook-type-reused.py
>  TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
> +TESTS += test/pacman/tests/hook-when-reused.py
>  TESTS += test/pacman/tests/ignore001.py
>  TESTS += test/pacman/tests/ignore002.py
>  TESTS += test/pacman/tests/ignore003.py
> diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
> new file mode 100644
> index 00000000..87637e80
> --- /dev/null
> +++ b/test/pacman/tests/hook-description-reused.py
> @@ -0,0 +1,23 @@
> +self.description = "Hook using multiple 'Description's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        Description = lala
> +        Description = foobar
> +        When = PreTransaction
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
> diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
> new file mode 100644
> index 00000000..38423177
> --- /dev/null
> +++ b/test/pacman/tests/hook-exec-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple 'Exec's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PostTransaction
> +        Exec = /bin/date
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
> diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
> new file mode 100644
> index 00000000..472c8caf
> --- /dev/null
> +++ b/test/pacman/tests/hook-type-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple 'Type's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Type = File
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PostTransaction
> +        Exec = /bin/date
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
> diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
> new file mode 100644
> index 00000000..85a93db8
> --- /dev/null
> +++ b/test/pacman/tests/hook-when-reused.py
> @@ -0,0 +1,22 @@
> +self.description = "Hook using multiple 'When's"
> +
> +self.add_hook("hook",
> +        """
> +        [Trigger]
> +        Type = Package
> +        Operation = Install
> +        Target = foo
> +
> +        [Action]
> +        When = PreTransaction
> +        Exec = /bin/date
> +        When = PostTransaction
> +        """);
> +
> +sp = pmpkg("foo")
> +self.addpkg2db("sync", sp)
> +
> +self.args = "-S foo"
> +
> +self.addrule("PACMAN_RETCODE=0")
> +self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
> --
> 2.11.0
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] hooks: warn if reassignment overwrites previous setting

Allan McRae
On 15/01/17 00:39, Andrew Gregory wrote:

> On 01/02/17 at 04:19pm, Stefan Klinger wrote:
>> hooks: complain if multiple Exec options in alpm hook
>>
>> hooks: test for reassign Exec
>>
>> hook: Make reassignment to Exec yield a warning
>>
>> hooks: more consistency checks in hook definition
>>
>> hooks: fixed typo in test descriptions
>>
>> hooks: refactored warnings into one common message
>>
>> hooks: using tabs for indentation
>>
>> fixed 2 space leaks when overwriting hook definitions
>>
>> hook: better macro definitions, more consistent usage of them
>> Signed-off-by: Stefan Klinger <[hidden email]>
>> ---
>>  lib/libalpm/hook.c                           | 58 ++++++++++++++--------------
>>  test/pacman/tests/TESTS                      |  4 ++
>>  test/pacman/tests/hook-description-reused.py | 23 +++++++++++
>>  test/pacman/tests/hook-exec-reused.py        | 22 +++++++++++
>>  test/pacman/tests/hook-type-reused.py        | 22 +++++++++++
>>  test/pacman/tests/hook-when-reused.py        | 22 +++++++++++
>>  6 files changed, 123 insertions(+), 28 deletions(-)
>>  create mode 100644 test/pacman/tests/hook-description-reused.py
>>  create mode 100644 test/pacman/tests/hook-exec-reused.py
>>  create mode 100644 test/pacman/tests/hook-type-reused.py
>>  create mode 100644 test/pacman/tests/hook-when-reused.py
>>
>> diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
>> index 4ec2a906..fdb6d475 100644
>> --- a/lib/libalpm/hook.c
>> +++ b/lib/libalpm/hook.c
>> @@ -30,6 +30,10 @@
>>  #include "trans.h"
>>  #include "util.h"
>>  
>> +
>> +#define error(...) do { _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1; } while (0)
>> +#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__)
>
> The error macro was written specifically for the section of code where
> it was used.  Using it elsewhere causes those functions to return
> early.  They were specifically written not to so that we could present
> all relevant errors to the user at once.
>

In addition, please label resubmitted patches as "v2" in the subject
line, and don't expand the scope of the patch in a revision (unless
specifically asked to).  Two patches would have been better, because I
could have accepted the first one with the fixes I had asked for.

Allan
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] hooks: Complain if hook parameters are overwritten. Fixed 2 space leaks.

Stefan Klinger
In reply to this post by Andrew Gregory
Signed-off-by: Stefan Klinger <[hidden email]>
---
 lib/libalpm/hook.c                           | 16 ++++++++++++++++
 test/pacman/tests/TESTS                      |  4 ++++
 test/pacman/tests/hook-description-reused.py | 23 +++++++++++++++++++++++
 test/pacman/tests/hook-exec-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-type-reused.py        | 22 ++++++++++++++++++++++
 test/pacman/tests/hook-when-reused.py        | 22 ++++++++++++++++++++++
 6 files changed, 109 insertions(+)
 create mode 100644 test/pacman/tests/hook-description-reused.py
 create mode 100644 test/pacman/tests/hook-exec-reused.py
 create mode 100644 test/pacman/tests/hook-type-reused.py
 create mode 100644 test/pacman/tests/hook-when-reused.py

diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c
index 4ec2a906..865c11d8 100644
--- a/lib/libalpm/hook.c
+++ b/lib/libalpm/hook.c
@@ -267,6 +267,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  struct _alpm_hook_t *hook = ctx->hook;
 
 #define error(...) _alpm_log(handle, ALPM_LOG_ERROR, __VA_ARGS__); return 1;
+#define warning(...) _alpm_log(handle, ALPM_LOG_WARNING, __VA_ARGS__);
 
  if(!section && !key) {
  error(_("error while reading hook %s: %s\n"), file, strerror(errno));
@@ -296,6 +297,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Type") == 0) {
+ if(t->type != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Type");
+ }
  if(strcmp(value, "Package") == 0) {
  t->type = ALPM_HOOK_TYPE_PACKAGE;
  } else if(strcmp(value, "File") == 0) {
@@ -312,6 +316,9 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
  } else if(strcmp(section, "Action") == 0) {
  if(strcmp(key, "When") == 0) {
+ if(hook->when != 0) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "When");
+ }
  if(strcmp(value, "PreTransaction") == 0) {
  hook->when = ALPM_HOOK_PRE_TRANSACTION;
  } else if(strcmp(value, "PostTransaction") == 0) {
@@ -320,6 +327,10 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
  }
  } else if(strcmp(key, "Description") == 0) {
+ if(hook->desc != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Description");
+ FREE(hook->desc);
+ }
  STRDUP(hook->desc, value, return 1);
  } else if(strcmp(key, "Depends") == 0) {
  char *val;
@@ -330,6 +341,10 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  } else if(strcmp(key, "NeedsTargets") == 0) {
  hook->needs_targets = 1;
  } else if(strcmp(key, "Exec") == 0) {
+ if(hook->cmd != NULL) {
+ warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec");
+ _alpm_wordsplit_free(hook->cmd);
+ }
  if((hook->cmd = _alpm_wordsplit(value)) == NULL) {
  if(errno == EINVAL) {
  error(_("hook %s line %d: invalid value %s\n"), file, line, value);
@@ -344,6 +359,7 @@ static int _alpm_hook_parse_cb(const char *file, int line,
  }
 
 #undef error
+#undef warning
 
  return 0;
 }
diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS
index 309eb17e..a9b4288c 100644
--- a/test/pacman/tests/TESTS
+++ b/test/pacman/tests/TESTS
@@ -55,6 +55,8 @@ TESTS += test/pacman/tests/fileconflict030.py
 TESTS += test/pacman/tests/fileconflict031.py
 TESTS += test/pacman/tests/fileconflict032.py
 TESTS += test/pacman/tests/hook-abortonfail.py
+TESTS += test/pacman/tests/hook-description-reused.py
+TESTS += test/pacman/tests/hook-exec-reused.py
 TESTS += test/pacman/tests/hook-exec-with-arguments.py
 TESTS += test/pacman/tests/hook-file-change-packages.py
 TESTS += test/pacman/tests/hook-file-remove-trigger-match.py
@@ -65,7 +67,9 @@ TESTS += test/pacman/tests/hook-pkg-postinstall-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-remove-trigger-match.py
 TESTS += test/pacman/tests/hook-pkg-upgrade-trigger-match.py
 TESTS += test/pacman/tests/hook-target-list.py
+TESTS += test/pacman/tests/hook-type-reused.py
 TESTS += test/pacman/tests/hook-upgrade-trigger-no-match.py
+TESTS += test/pacman/tests/hook-when-reused.py
 TESTS += test/pacman/tests/ignore001.py
 TESTS += test/pacman/tests/ignore002.py
 TESTS += test/pacman/tests/ignore003.py
diff --git a/test/pacman/tests/hook-description-reused.py b/test/pacman/tests/hook-description-reused.py
new file mode 100644
index 00000000..87637e80
--- /dev/null
+++ b/test/pacman/tests/hook-description-reused.py
@@ -0,0 +1,23 @@
+self.description = "Hook using multiple 'Description's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        Description = lala
+        Description = foobar
+        When = PreTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Description")
diff --git a/test/pacman/tests/hook-exec-reused.py b/test/pacman/tests/hook-exec-reused.py
new file mode 100644
index 00000000..38423177
--- /dev/null
+++ b/test/pacman/tests/hook-exec-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Exec's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Exec")
diff --git a/test/pacman/tests/hook-type-reused.py b/test/pacman/tests/hook-type-reused.py
new file mode 100644
index 00000000..472c8caf
--- /dev/null
+++ b/test/pacman/tests/hook-type-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'Type's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Type = File
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PostTransaction
+        Exec = /bin/date
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of Type")
diff --git a/test/pacman/tests/hook-when-reused.py b/test/pacman/tests/hook-when-reused.py
new file mode 100644
index 00000000..85a93db8
--- /dev/null
+++ b/test/pacman/tests/hook-when-reused.py
@@ -0,0 +1,22 @@
+self.description = "Hook using multiple 'When's"
+
+self.add_hook("hook",
+        """
+        [Trigger]
+        Type = Package
+        Operation = Install
+        Target = foo
+
+        [Action]
+        When = PreTransaction
+        Exec = /bin/date
+        When = PostTransaction
+        """);
+
+sp = pmpkg("foo")
+self.addpkg2db("sync", sp)
+
+self.args = "-S foo"
+
+self.addrule("PACMAN_RETCODE=0")
+self.addrule("PACMAN_OUTPUT=warning.*overwriting previous definition of When")
--
2.16.2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] hooks: Complain if hook parameters are overwritten. Fixed 2 space leaks.

Stefan Klinger
Any feedback on this?


--
http://stefan-klinger.de                                      o/X
Send plain text messages only, not exceeding 32kB.            /\/
                                                                \
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] hooks: Complain if hook parameters are overwritten. Fixed 2 space leaks.

Eli Schwartz-2
On 03/08/2018 07:57 AM, [hidden email] wrote:
> Any feedback on this?

https://lists.archlinux.org/pipermail/pacman-dev/2018-March/022366.html

You're not being ignored, we all are. :D

Just wait until Allan has free time to catch up on the patch queue.

--
Eli Schwartz
Bug Wrangler and Trusted User


signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] hooks: Complain if hook parameters are overwritten. Fixed 2 space leaks.

Allan McRae
In reply to this post by Stefan Klinger
On 08/03/18 22:57, [hidden email] wrote:
> Any feedback on this?

At a glance, the patch looks good and addresses the concerns raised for
the original patch.

I'll get to pulling it soon...

A
12