[pacman-dev] [PATCH 2/2] Update the question callback
Allan McRae
allan at archlinux.org
Thu May 22 06:41:01 EDT 2014
On 16/03/14 22:40, Olivier Brunel wrote:
> Much like with events, instead of using a bunch of void* arguments for
> all questions, we now send one pointer to an alpm_question_t struct.
> This contains the type of question that was triggered.
>
> With this information, the pointer can then be typecasted to the
> question-specific struct in order to get additional arguments.
>
> Signed-off-by: Olivier Brunel <jjk at jjacky.com>
Finally getting to this...
> ---
> lib/libalpm/alpm.h | 104 ++++++++++++++++++++++++++++++++++++++++++-----
> lib/libalpm/deps.c | 38 +++++++++++-------
> lib/libalpm/handle.h | 4 +-
> lib/libalpm/signing.c | 12 ++++--
> lib/libalpm/sync.c | 60 +++++++++++++++++----------
> src/pacman/callback.c | 109 +++++++++++++++++++++++++++-----------------------
> src/pacman/callback.h | 3 +-
> 7 files changed, 227 insertions(+), 103 deletions(-)
>
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 66bb5f9..9a1be48 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -567,23 +567,107 @@ typedef struct _alpm_event_pacorig_created_t {
> typedef void (*alpm_cb_event)(alpm_event_t *);
>
> /**
> - * Questions.
> + * Type of questions.
> * Unlike the events or progress enumerations, this enum has bitmask values
> * so a frontend can use a bitmask map to supply preselected answers to the
> * different types of questions.
> */
> -typedef enum _alpm_question_t {
> - ALPM_QUESTION_INSTALL_IGNOREPKG = 1,
> - ALPM_QUESTION_REPLACE_PKG = (1 << 1),
> - ALPM_QUESTION_CONFLICT_PKG = (1 << 2),
> - ALPM_QUESTION_CORRUPTED_PKG = (1 << 3),
> - ALPM_QUESTION_REMOVE_PKGS = (1 << 4),
> - ALPM_QUESTION_SELECT_PROVIDER = (1 << 5),
> - ALPM_QUESTION_IMPORT_KEY = (1 << 6)
> +typedef enum _alpm_question_type_t {
> + ALPM_QUESTION_INSTALL_IGNOREPKG = (1 << 0),
> + ALPM_QUESTION_REPLACE_PKG = (1 << 1),
> + ALPM_QUESTION_CONFLICT_PKG = (1 << 2),
> + ALPM_QUESTION_CORRUPTED_PKG = (1 << 3),
> + ALPM_QUESTION_REMOVE_PKGS = (1 << 4),
> + ALPM_QUESTION_SELECT_PROVIDER = (1 << 5),
> + ALPM_QUESTION_IMPORT_KEY = (1 << 6)
Do not justify the values. This results in unnecessary churn when a
longer member is added. Stick to the single space as in the original.
We are trying stop this throughout the code base.
> +} alpm_question_type_t;
> +
> +/**
> + * Questions.
> + * This is a generic struct this is passed to the callback, that allows the
> + * frontend to know which type of question was triggered. It is then possible to
> + * typecast the pointer to the right structure, in order to access
> + * question-specific data. */
> +typedef struct _alpm_question_t {
> + /** Type of question. */
> + alpm_question_type_t type;
> + /** Answer. */
> + int answer;
> } alpm_question_t;
>
> +typedef struct _alpm_question_install_ignorepkg_t {
> + /** Type of questions. */
question
> + alpm_question_type_t type;
> + /** Answer: whether or not to install pkg anyway. */
> + int install;
> + /* Package in IgnorePkg/IgnoreGroup. */
> + alpm_pkg_t *pkg;
> +} alpm_question_install_ignorepkg_t;
> +
> +typedef struct _alpm_question_replace_t {
> + /** Type of questions. */
question (and throughout)
> + alpm_question_type_t type;
> + /** Answer: whether or not to replace oldpkg with newpkg. */
> + int replace;
> + /* Package to be replaced. */
> + alpm_pkg_t *oldpkg;
> + /* Package to replace with. */
> + alpm_pkg_t *newpkg;
> + /* DB of newpkg */
> + alpm_db_t *newdb;
> +} alpm_question_replace_t;
> +
> +typedef struct _alpm_question_conflict_t {
> + /** Type of questions. */
> + alpm_question_type_t type;
> + /** Answer: whether or not to remove conflict->package2. */
> + int remove;
> + /** Conflict info. */
> + alpm_conflict_t *conflict;
> +} alpm_question_conflict_t;
> +
> +typedef struct _alpm_question_corrupted_t {
> + /** Type of questions. */
> + alpm_question_type_t type;
> + /** Answer: whether or not to remove filepath. */
> + int remove;
> + /** Filename to remove */
> + const char *filepath;
> + /** Error code indicating the reason for package invalidity */
> + alpm_errno_t reason;
> +} alpm_question_corrupted_t;
> +
> +typedef struct _alpm_question_remove_pkgs_t {
> + /** Type of questions. */
> + alpm_question_type_t type;
> + /** Answer: whether or not to skip packages. */
> + int skip;
> + /** List of alpm_pkg_t* with unresolved dependencies. */
> + alpm_list_t *packages;
> +} alpm_question_remove_pkgs_t;
> +
> +typedef struct _alpm_question_select_provider_t {
> + /** Type of questions. */
> + alpm_question_type_t type;
> + /** Answer: which provider to use (index from providers). */
> + int use_index;
> + /** List of alpm_pkg_t* as possible providers. */
> + alpm_list_t *providers;
> + /** What providers provide for. */
> + alpm_depend_t *depend;
> +} alpm_question_select_provider_t;
> +
> +typedef struct _alpm_question_import_key_t {
> + /** Type of questions. */
> + alpm_question_type_t type;
> + /** Answer: whether or not to import key. */
> + int import;
> + /** The key to import. */
> + alpm_pgpkey_t *key;
> +} alpm_question_import_key_t;
> +
> /** Question callback */
> -typedef void (*alpm_cb_question)(alpm_question_t, void *, void *, void *, int *);
> +typedef void (*alpm_cb_question)(alpm_question_t *);
>
> /** Progress */
> typedef enum _alpm_progress_t {
> diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c
> index b3de1b0..1cbbc5f 100644
> --- a/lib/libalpm/deps.c
> +++ b/lib/libalpm/deps.c
> @@ -640,15 +640,18 @@ static alpm_pkg_t *resolvedep(alpm_handle_t *handle, alpm_depend_t *dep,
> if(pkg && _alpm_depcmp_literal(pkg, dep)
> && !alpm_pkg_find(excluding, pkg->name)) {
> if(alpm_pkg_should_ignore(handle, pkg)) {
> - int install = 0;
> + alpm_question_install_ignorepkg_t question = {
> + .type = ALPM_QUESTION_INSTALL_IGNOREPKG,
> + .install = 0,
> + .pkg = pkg
> + };
> if(prompt) {
> - QUESTION(handle, ALPM_QUESTION_INSTALL_IGNOREPKG, pkg,
> - NULL, NULL, &install);
> + QUESTION(handle, &question);
> } else {
> _alpm_log(handle, ALPM_LOG_WARNING, _("ignoring package %s-%s\n"),
> pkg->name, pkg->version);
> }
> - if(!install) {
> + if(!question.install) {
> ignored = 1;
> continue;
> }
> @@ -669,15 +672,18 @@ static alpm_pkg_t *resolvedep(alpm_handle_t *handle, alpm_depend_t *dep,
> if(pkg->name_hash != dep->name_hash && _alpm_depcmp(pkg, dep)
> && !alpm_pkg_find(excluding, pkg->name)) {
> if(alpm_pkg_should_ignore(handle, pkg)) {
> - int install = 0;
> + alpm_question_install_ignorepkg_t question = {
> + .type = ALPM_QUESTION_INSTALL_IGNOREPKG,
> + .install = 0,
> + .pkg = pkg
> + };
> if(prompt) {
> - QUESTION(handle, ALPM_QUESTION_INSTALL_IGNOREPKG,
> - pkg, NULL, NULL, &install);
> + QUESTION(handle, &question);
> } else {
> _alpm_log(handle, ALPM_LOG_WARNING, _("ignoring package %s-%s\n"),
> pkg->name, pkg->version);
> }
> - if(!install) {
> + if(!question.install) {
> ignored = 1;
> continue;
> }
> @@ -700,15 +706,19 @@ static alpm_pkg_t *resolvedep(alpm_handle_t *handle, alpm_depend_t *dep,
> }
> count = alpm_list_count(providers);
> if(count >= 1) {
> - /* default to first provider if there is no QUESTION callback */
> - int idx = 0;
> + alpm_question_select_provider_t question = {
> + .type = ALPM_QUESTION_SELECT_PROVIDER,
> + /* default to first provider if there is no QUESTION callback */
> + .use_index = 0,
> + .providers = providers,
> + .depend = dep
> + };
> if(count > 1) {
> /* if there is more than one provider, we ask the user */
> - QUESTION(handle, ALPM_QUESTION_SELECT_PROVIDER,
> - providers, dep, NULL, &idx);
> + QUESTION(handle, &question);
> }
> - if(idx >= 0 && idx < count) {
> - alpm_list_t *nth = alpm_list_nth(providers, idx);
> + if(question.use_index >= 0 && question.use_index < count) {
> + alpm_list_t *nth = alpm_list_nth(providers, question.use_index);
> alpm_pkg_t *pkg = nth->data;
> alpm_list_free(providers);
> return pkg;
> diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h
> index 27241ea..85c64f6 100644
> --- a/lib/libalpm/handle.h
> +++ b/lib/libalpm/handle.h
> @@ -37,10 +37,10 @@ do { \
> (h)->eventcb((alpm_event_t *) (e)); \
> } \
> } while(0)
> -#define QUESTION(h, q, d1, d2, d3, r) \
> +#define QUESTION(h, q) \
> do { \
> if((h)->questioncb) { \
> - (h)->questioncb(q, d1, d2, d3, r); \
> + (h)->questioncb((alpm_question_t *) (q)); \
> } \
> } while(0)
> #define PROGRESS(h, e, p, per, n, r) \
> diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c
> index caacd24..cf3010f 100644
> --- a/lib/libalpm/signing.c
> +++ b/lib/libalpm/signing.c
> @@ -413,7 +413,7 @@ gpg_error:
> */
> int _alpm_key_import(alpm_handle_t *handle, const char *fpr)
> {
> - int answer = 0, ret = -1;
> + int ret = -1;
> alpm_pgpkey_t fetch_key;
> memset(&fetch_key, 0, sizeof(fetch_key));
>
> @@ -421,9 +421,13 @@ int _alpm_key_import(alpm_handle_t *handle, const char *fpr)
> _alpm_log(handle, ALPM_LOG_DEBUG,
> "unknown key, found %s on keyserver\n", fetch_key.uid);
> if(!_alpm_access(handle, handle->gpgdir, "pubring.gpg", W_OK)) {
> - QUESTION(handle, ALPM_QUESTION_IMPORT_KEY,
> - &fetch_key, NULL, NULL, &answer);
> - if(answer) {
> + alpm_question_import_key_t question = {
> + .type = ALPM_QUESTION_IMPORT_KEY,
> + .import = 0,
> + .key = &fetch_key
> + };
> + QUESTION(handle, &question);
> + if(question.import) {
> if(key_import(handle, &fetch_key) == 0) {
> ret = 0;
> } else {
> diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> index 718f9af..40e9033 100644
> --- a/lib/libalpm/sync.c
> +++ b/lib/libalpm/sync.c
> @@ -145,7 +145,13 @@ static alpm_list_t *check_replacers(alpm_handle_t *handle, alpm_pkg_t *lpkg,
> }
> }
> if(found) {
> - int doreplace = 0;
> + alpm_question_replace_t question = {
> + .type = ALPM_QUESTION_REPLACE_PKG,
> + .replace = 0,
> + .oldpkg = lpkg,
> + .newpkg = spkg,
> + .newdb = sdb
> + };
> alpm_pkg_t *tpkg;
> /* check IgnorePkg/IgnoreGroup */
> if(alpm_pkg_should_ignore(handle, spkg)
> @@ -156,9 +162,8 @@ static alpm_list_t *check_replacers(alpm_handle_t *handle, alpm_pkg_t *lpkg,
> continue;
> }
>
> - QUESTION(handle, ALPM_QUESTION_REPLACE_PKG, lpkg, spkg,
> - sdb->treename, &doreplace);
> - if(!doreplace) {
> + QUESTION(handle, &question);
> + if(!question.replace) {
> continue;
> }
>
> @@ -276,11 +281,14 @@ alpm_list_t SYMEXPORT *alpm_find_group_pkgs(alpm_list_t *dbs,
> continue;
> }
> if(alpm_pkg_should_ignore(db->handle, pkg)) {
> + alpm_question_install_ignorepkg_t question = {
> + .type = ALPM_QUESTION_INSTALL_IGNOREPKG,
> + .install = 0,
> + .pkg = pkg
> + };
> ignorelist = alpm_list_add(ignorelist, pkg);
> - int install = 0;
> - QUESTION(db->handle, ALPM_QUESTION_INSTALL_IGNOREPKG, pkg,
> - NULL, NULL, &install);
> - if(!install)
> + QUESTION(db->handle, &question);
> + if(!question.install)
> continue;
> }
> if(!alpm_pkg_find(pkgs, pkg->name)) {
> @@ -443,10 +451,13 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data)
> /* If there were unresolvable top-level packages, prompt the user to
> see if they'd like to ignore them rather than failing the sync */
> if(unresolvable != NULL) {
> - int remove_unresolvable = 0;
> - QUESTION(handle, ALPM_QUESTION_REMOVE_PKGS, unresolvable,
> - NULL, NULL, &remove_unresolvable);
> - if(remove_unresolvable) {
> + alpm_question_remove_pkgs_t question = {
> + .type = ALPM_QUESTION_REMOVE_PKGS,
> + .skip = 0,
> + .packages = unresolvable
> + };
> + QUESTION(handle, &question);
> + if(question.skip) {
> /* User wants to remove the unresolvable packages from the
> transaction. The packages will be removed from the actual
> transaction when the transaction packages are replaced with a
> @@ -560,8 +571,12 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data)
> deps = _alpm_outerconflicts(handle->db_local, trans->add);
>
> for(i = deps; i; i = i->next) {
> + alpm_question_conflict_t question = {
> + .type = ALPM_QUESTION_CONFLICT_PKG,
> + .remove = 0,
> + .conflict = i->data
> + };
> alpm_conflict_t *conflict = i->data;
> - int doremove = 0;
> int found = 0;
>
> /* if conflict->package2 (the local package) is not elected for removal,
> @@ -579,9 +594,8 @@ int _alpm_sync_prepare(alpm_handle_t *handle, alpm_list_t **data)
> _alpm_log(handle, ALPM_LOG_DEBUG, "package '%s' conflicts with '%s'\n",
> conflict->package1, conflict->package2);
>
> - QUESTION(handle, ALPM_QUESTION_CONFLICT_PKG, conflict->package1,
> - conflict->package2, conflict->reason->name, &doremove);
> - if(doremove) {
> + QUESTION(handle, &question);
> + if(question.remove) {
> /* append to the removes list */
> alpm_pkg_t *sync = alpm_pkg_find(trans->add, conflict->package1);
> alpm_pkg_t *local = _alpm_db_get_pkgfromcache(handle->db_local, conflict->package2);
> @@ -790,13 +804,17 @@ static int apply_deltas(alpm_handle_t *handle)
> static int prompt_to_delete(alpm_handle_t *handle, const char *filepath,
> alpm_errno_t reason)
> {
> - int doremove = 0;
> - QUESTION(handle, ALPM_QUESTION_CORRUPTED_PKG, (char *)filepath,
> - &reason, NULL, &doremove);
> - if(doremove) {
> + alpm_question_corrupted_t question = {
> + .type = ALPM_QUESTION_CORRUPTED_PKG,
> + .remove = 0,
> + .filepath = filepath,
> + .reason = reason
> + };
> + QUESTION(handle, &question);
> + if(question.remove) {
> unlink(filepath);
> }
> - return doremove;
> + return question.remove;
> }
>
> static int validate_deltas(alpm_handle_t *handle, alpm_list_t *deltas)
> diff --git a/src/pacman/callback.c b/src/pacman/callback.c
> index 340a3a1..b374fa9 100644
> --- a/src/pacman/callback.c
> +++ b/src/pacman/callback.c
> @@ -361,55 +361,61 @@ void cb_event(alpm_event_t *event)
> }
>
> /* callback to handle questions from libalpm transactions (yes/no) */
> -/* TODO this is one of the worst ever functions written. void *data ? wtf */
> -void cb_question(alpm_question_t event, void *data1, void *data2,
> - void *data3, int *response)
> +void cb_question(alpm_question_t *question)
> {
> if(config->print) {
> - if(event == ALPM_QUESTION_INSTALL_IGNOREPKG) {
> - *response = 1;
> + if(question->type == ALPM_QUESTION_INSTALL_IGNOREPKG) {
> + question->answer = 1;
> } else {
> - *response = 0;
> + question->answer = 0;
> }
> return;
> }
> - switch(event) {
> + switch(question->type) {
> case ALPM_QUESTION_INSTALL_IGNOREPKG:
> - if(!config->op_s_downloadonly) {
> - *response = yesno(_("%s is in IgnorePkg/IgnoreGroup. Install anyway?"),
> - alpm_pkg_get_name(data1));
> - } else {
> - *response = 1;
> + {
> + alpm_question_install_ignorepkg_t *q = (alpm_question_install_ignorepkg_t *) question;
> + if(!config->op_s_downloadonly) {
> + q->install = yesno(_("%s is in IgnorePkg/IgnoreGroup. Install anyway?"),
> + alpm_pkg_get_name(q->pkg));
> + } else {
> + q->install = 1;
> + }
> }
> break;
> case ALPM_QUESTION_REPLACE_PKG:
> - *response = yesno(_("Replace %s with %s/%s?"),
> - alpm_pkg_get_name(data1),
> - (char *)data3,
> - alpm_pkg_get_name(data2));
> + {
> + alpm_question_replace_t *q = (alpm_question_replace_t *) question;
> + q->replace = yesno(_("Replace %s with %s/%s?"),
> + alpm_pkg_get_name(q->oldpkg),
> + alpm_db_get_name(q->newdb),
> + alpm_pkg_get_name(q->newpkg));
> + }
> break;
> case ALPM_QUESTION_CONFLICT_PKG:
> - /* data parameters: target package, local package, conflict (strings) */
> - /* print conflict only if it contains new information */
I'd like to keep this comment
> - if(strcmp(data1, data3) == 0 || strcmp(data2, data3) == 0) {
> - *response = noyes(_("%s and %s are in conflict. Remove %s?"),
> - (char *)data1,
> - (char *)data2,
> - (char *)data2);
> - } else {
> - *response = noyes(_("%s and %s are in conflict (%s). Remove %s?"),
> - (char *)data1,
> - (char *)data2,
> - (char *)data3,
> - (char *)data2);
> + {
> + alpm_question_conflict_t *q = (alpm_question_conflict_t *) question;
> + if(strcmp(q->conflict->package1, q->conflict->reason->name) == 0
> + || strcmp(q->conflict->package2, q->conflict->reason->name) == 0) {
> + q->remove = noyes(_("%s and %s are in conflict. Remove %s?"),
> + q->conflict->package1,
> + q->conflict->package2,
> + q->conflict->package2);
> + } else {
> + q->remove = noyes(_("%s and %s are in conflict (%s). Remove %s?"),
> + q->conflict->package1,
> + q->conflict->package2,
> + q->conflict->reason->name,
> + q->conflict->package2);
> + }
> }
> break;
> case ALPM_QUESTION_REMOVE_PKGS:
> {
> - alpm_list_t *unresolved = data1;
> + alpm_question_remove_pkgs_t *q = (alpm_question_remove_pkgs_t *) question;
> alpm_list_t *namelist = NULL, *i;
> size_t count = 0;
> - for(i = unresolved; i; i = i->next) {
> + for(i = q->packages; i; i = i->next) {
> namelist = alpm_list_add(namelist,
> (char *)alpm_pkg_get_name(i->data));
> count++;
> @@ -420,7 +426,7 @@ void cb_question(alpm_question_t event, void *data1, void *data2,
> count));
> list_display(" ", namelist, getcols(fileno(stdout)));
> printf("\n");
> - *response = noyes(_n(
> + q->skip = noyes(_n(
> "Do you want to skip the above package for this upgrade?",
> "Do you want to skip the above packages for this upgrade?",
> count));
> @@ -429,43 +435,46 @@ void cb_question(alpm_question_t event, void *data1, void *data2,
> break;
> case ALPM_QUESTION_SELECT_PROVIDER:
> {
> - alpm_list_t *providers = data1;
> - size_t count = alpm_list_count(providers);
> - char *depstring = alpm_dep_compute_string((alpm_depend_t *)data2);
> + alpm_question_select_provider_t *q = (alpm_question_select_provider_t *) question;
> + size_t count = alpm_list_count(q->providers);
> + char *depstring = alpm_dep_compute_string(q->depend);
> colon_printf(_("There are %zd providers available for %s:\n"), count,
> depstring);
> free(depstring);
> - select_display(providers);
> - *response = select_question(count);
> + select_display(q->providers);
> + q->use_index = select_question(count);
> }
> break;
> case ALPM_QUESTION_CORRUPTED_PKG:
> - *response = yesno(_("File %s is corrupted (%s).\n"
> - "Do you want to delete it?"),
> - (char *)data1,
> - alpm_strerror(*(alpm_errno_t *)data2));
> + {
> + alpm_question_corrupted_t *q = (alpm_question_corrupted_t *) question;
> + q->remove = yesno(_("File %s is corrupted (%s).\n"
> + "Do you want to delete it?"),
> + q->filepath,
> + alpm_strerror(q->reason));
> + }
> break;
> case ALPM_QUESTION_IMPORT_KEY:
> {
> - alpm_pgpkey_t *key = data1;
> + alpm_question_import_key_t *q = (alpm_question_import_key_t *) question;
> char created[12];
> - time_t time = (time_t)key->created;
> + time_t time = (time_t)q->key->created;
> strftime(created, 12, "%Y-%m-%d", localtime(&time));
>
> - if(key->revoked) {
> - *response = yesno(_("Import PGP key %d%c/%s, \"%s\", created: %s (revoked)?"),
> - key->length, key->pubkey_algo, key->fingerprint, key->uid, created);
> + if(q->key->revoked) {
> + q->import = yesno(_("Import PGP key %d%c/%s, \"%s\", created: %s (revoked)?"),
> + q->key->length, q->key->pubkey_algo, q->key->fingerprint, q->key->uid, created);
> } else {
> - *response = yesno(_("Import PGP key %d%c/%s, \"%s\", created: %s?"),
> - key->length, key->pubkey_algo, key->fingerprint, key->uid, created);
> + q->import = yesno(_("Import PGP key %d%c/%s, \"%s\", created: %s?"),
> + q->key->length, q->key->pubkey_algo, q->key->fingerprint, q->key->uid, created);
> }
> }
> break;
> }
> if(config->noask) {
> - if(config->ask & event) {
> + if(config->ask & question->type) {
> /* inverse the default answer */
> - *response = !*response;
> + question->answer = !question->answer;
> }
> }
> }
> diff --git a/src/pacman/callback.h b/src/pacman/callback.h
> index f16f7fc..e4941fc 100644
> --- a/src/pacman/callback.h
> +++ b/src/pacman/callback.h
> @@ -28,8 +28,7 @@
> void cb_event(alpm_event_t *event);
>
> /* callback to handle questions from libalpm (yes/no) */
> -void cb_question(alpm_question_t event, void *data1, void *data2,
> - void *data3, int *response);
> +void cb_question(alpm_question_t* question);
>
> /* callback to handle display of progress */
> void cb_progress(alpm_progress_t event, const char *pkgname, int percent,
>
More information about the pacman-dev
mailing list