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

Hi On Mon, Jan 2, 2017 at 4:19 PM, 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 --- 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

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 --- 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.

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

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 {

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

Signed-off-by: Stefan Klinger --- 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