diff mbox

[pacman-dev,v2] hooks: Complain if hook parameters are overwritten. Fixed 2 space leaks.

Message ID E1eq1PV-0002m3-NU@smtprelay02.ispgateway.de
State Accepted, archived
Headers show

Commit Message

Stefan Klinger Feb. 25, 2018, 5:36 p.m. UTC
Signed-off-by: Stefan Klinger <git@stefan-klinger.de>
---
 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

Comments

Stefan Klinger March 8, 2018, 12:57 p.m. UTC | #1
Any feedback on this?
Eli Schwartz March 8, 2018, 1:04 p.m. UTC | #2
On 03/08/2018 07:57 AM, git@stefan-klinger.de 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.
Allan McRae March 8, 2018, 1:24 p.m. UTC | #3
On 08/03/18 22:57, git@stefan-klinger.de 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
Allan McRae March 14, 2018, 3:03 a.m. UTC | #4
On 26/02/18 03:36, Stefan Klinger wrote:
> Signed-off-by: Stefan Klinger <git@stefan-klinger.de>
> ---
>  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


Thanks - patch looks good.   Even better due to the inclusion of test
suite additions!

A
diff mbox

Patch

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")