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 <git@stefan-klinger.de> --- 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