[pacman-dev] [PATCH] hooks: warn if reassignment overwrites previous setting
Andrew Gregory
andrew.gregory.8 at gmail.com
Sat Jan 14 14:39:19 UTC 2017
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 at 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
More information about the pacman-dev
mailing list