Re: [pacman-dev] [PATCH v3 2/2] Update the question callback

On 05/31/14 at 07:28pm, 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 union. This contains the type of question that was triggered.

With this information, a question-specific struct can be accessed in order to get additional arguments.

Signed-off-by: Olivier Brunel --- This uses for alpm_quesion_t an union with all question-specific struct, I'm thinking this should resolve the issue reported with clang.

I don't actually have clang here though, Andrew could you try it and let me know whether it solves the issue or not, please?

If it does, I'll do a patch with similar changes for events.

-j

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 | 110 +++++++++++++++++++++++++++----------------------- src/pacman/callback.h | 3 +- 7 files changed, 234 insertions(+), 97 deletions(-)

This does silence Clang's warnings about casting. I don't have much occasion to use unions personally; is it safe to read the question type directly from the union and to cast the union to the various question types rather than accessing them as fields? apg

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 66bb5f9..c81b1f2 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -567,23 +567,119 @@ 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, +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) +} alpm_question_type_t; + +typedef struct _alpm_question_any_t { + /** Type of question. */ + alpm_question_type_t type; + /** Answer. */ + int answer; +} alpm_question_any_t; + +typedef struct _alpm_question_install_ignorepkg_t { + /** Type of 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 question. */ + 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 question. */ + 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 question. */ + 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 question. */ + 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 question. */ + 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 question. */ + 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; + +/** + * Questions. + * This is an union passed to the callback, that allows the frontend to know + * which type of question was triggered (via type). It is then possible to + * typecast the pointer to the right structure, or use the union field, in order + * to access question-specific data. */ +typedef union _alpm_question_t { + alpm_question_type_t type; + alpm_question_any_t any; + alpm_question_install_ignorepkg_t install_ignorepkg; + alpm_question_replace_t replace; + alpm_question_conflict_t conflict; + alpm_question_corrupted_t corrupted; + alpm_question_remove_pkgs_t remove_pkgs; + alpm_question_select_provider_t select_provider; + alpm_question_import_key_t import_key; } alpm_question_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 a025b68..38e8f9e 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, @@ -582,9 +597,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); @@ -793,13 +807,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..cb4aaf0 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -361,55 +361,62 @@ 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->any.answer = 1; } else { - *response = 0; + question->any.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 */ - 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; + /* print conflict only if it contains new information */ + 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 +427,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 +436,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->any.answer = !question->any.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, -- 1.9.3