On 06/07/14 15:45, Andrew Gregory wrote:
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 <jjk@jjacky.com> --- 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?
I believe it is, yes. In fact it's something done frequently when dealing with e.g. XEvent or GdkEvent, both are union working similarly, where one can use the union field or typecast the pointer to the event-specific struct. Though I could change things to access union fields instead of typecasting if it is preferred? -j
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