[pacman-dev] [PATCH 1/2] alpm.h: Move alpm_errno_t definition up
Because for the new question types, we'll need to use alpm_errno_t let's move its definition up. Of course to do so, we also need to move that of alpm_handle_t as well, so move all opaque structures on top. Signed-off-by: Olivier Brunel <jjk@jjacky.com> --- lib/libalpm/alpm.h | 171 +++++++++++++++++++++++++++-------------------------- 1 file changed, 87 insertions(+), 84 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index b0adb95..66bb5f9 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -41,6 +41,93 @@ extern "C" { * Arch Linux Package Management library */ +/* + * Opaque Structures + */ +typedef struct __alpm_handle_t alpm_handle_t; +typedef struct __alpm_db_t alpm_db_t; +typedef struct __alpm_pkg_t alpm_pkg_t; +typedef struct __alpm_trans_t alpm_trans_t; + +/** @addtogroup alpm_api_errors Error Codes + * @{ + */ +typedef enum _alpm_errno_t { + ALPM_ERR_MEMORY = 1, + ALPM_ERR_SYSTEM, + ALPM_ERR_BADPERMS, + ALPM_ERR_NOT_A_FILE, + ALPM_ERR_NOT_A_DIR, + ALPM_ERR_WRONG_ARGS, + ALPM_ERR_DISK_SPACE, + /* Interface */ + ALPM_ERR_HANDLE_NULL, + ALPM_ERR_HANDLE_NOT_NULL, + ALPM_ERR_HANDLE_LOCK, + /* Databases */ + ALPM_ERR_DB_OPEN, + ALPM_ERR_DB_CREATE, + ALPM_ERR_DB_NULL, + ALPM_ERR_DB_NOT_NULL, + ALPM_ERR_DB_NOT_FOUND, + ALPM_ERR_DB_INVALID, + ALPM_ERR_DB_INVALID_SIG, + ALPM_ERR_DB_VERSION, + ALPM_ERR_DB_WRITE, + ALPM_ERR_DB_REMOVE, + /* Servers */ + ALPM_ERR_SERVER_BAD_URL, + ALPM_ERR_SERVER_NONE, + /* Transactions */ + ALPM_ERR_TRANS_NOT_NULL, + ALPM_ERR_TRANS_NULL, + ALPM_ERR_TRANS_DUP_TARGET, + ALPM_ERR_TRANS_NOT_INITIALIZED, + ALPM_ERR_TRANS_NOT_PREPARED, + ALPM_ERR_TRANS_ABORT, + ALPM_ERR_TRANS_TYPE, + ALPM_ERR_TRANS_NOT_LOCKED, + /* Packages */ + ALPM_ERR_PKG_NOT_FOUND, + ALPM_ERR_PKG_IGNORED, + ALPM_ERR_PKG_INVALID, + ALPM_ERR_PKG_INVALID_CHECKSUM, + ALPM_ERR_PKG_INVALID_SIG, + ALPM_ERR_PKG_MISSING_SIG, + ALPM_ERR_PKG_OPEN, + ALPM_ERR_PKG_CANT_REMOVE, + ALPM_ERR_PKG_INVALID_NAME, + ALPM_ERR_PKG_INVALID_ARCH, + ALPM_ERR_PKG_REPO_NOT_FOUND, + /* Signatures */ + ALPM_ERR_SIG_MISSING, + ALPM_ERR_SIG_INVALID, + /* Deltas */ + ALPM_ERR_DLT_INVALID, + ALPM_ERR_DLT_PATCHFAILED, + /* Dependencies */ + ALPM_ERR_UNSATISFIED_DEPS, + ALPM_ERR_CONFLICTING_DEPS, + ALPM_ERR_FILE_CONFLICTS, + /* Misc */ + ALPM_ERR_RETRIEVE, + ALPM_ERR_INVALID_REGEX, + /* External library errors */ + ALPM_ERR_LIBARCHIVE, + ALPM_ERR_LIBCURL, + ALPM_ERR_EXTERNAL_DOWNLOAD, + ALPM_ERR_GPGME +} alpm_errno_t; + +/** Returns the current error code from the handle. */ +alpm_errno_t alpm_errno(alpm_handle_t *handle); + +/** Returns the string corresponding to an error number. */ +const char *alpm_strerror(alpm_errno_t err); + +/* End of alpm_api_errors */ +/** @} */ + /** @addtogroup alpm_api Public API * The libalpm Public API * @{ @@ -143,11 +230,6 @@ typedef enum _alpm_sigvalidity_t { * Structures */ -typedef struct __alpm_handle_t alpm_handle_t; -typedef struct __alpm_db_t alpm_db_t; -typedef struct __alpm_pkg_t alpm_pkg_t; -typedef struct __alpm_trans_t alpm_trans_t; - /** Dependency */ typedef struct _alpm_depend_t { char *name; @@ -1345,85 +1427,6 @@ char *alpm_dep_compute_string(const alpm_depend_t *dep); char *alpm_compute_md5sum(const char *filename); char *alpm_compute_sha256sum(const char *filename); -/** @addtogroup alpm_api_errors Error Codes - * @{ - */ -typedef enum _alpm_errno_t { - ALPM_ERR_MEMORY = 1, - ALPM_ERR_SYSTEM, - ALPM_ERR_BADPERMS, - ALPM_ERR_NOT_A_FILE, - ALPM_ERR_NOT_A_DIR, - ALPM_ERR_WRONG_ARGS, - ALPM_ERR_DISK_SPACE, - /* Interface */ - ALPM_ERR_HANDLE_NULL, - ALPM_ERR_HANDLE_NOT_NULL, - ALPM_ERR_HANDLE_LOCK, - /* Databases */ - ALPM_ERR_DB_OPEN, - ALPM_ERR_DB_CREATE, - ALPM_ERR_DB_NULL, - ALPM_ERR_DB_NOT_NULL, - ALPM_ERR_DB_NOT_FOUND, - ALPM_ERR_DB_INVALID, - ALPM_ERR_DB_INVALID_SIG, - ALPM_ERR_DB_VERSION, - ALPM_ERR_DB_WRITE, - ALPM_ERR_DB_REMOVE, - /* Servers */ - ALPM_ERR_SERVER_BAD_URL, - ALPM_ERR_SERVER_NONE, - /* Transactions */ - ALPM_ERR_TRANS_NOT_NULL, - ALPM_ERR_TRANS_NULL, - ALPM_ERR_TRANS_DUP_TARGET, - ALPM_ERR_TRANS_NOT_INITIALIZED, - ALPM_ERR_TRANS_NOT_PREPARED, - ALPM_ERR_TRANS_ABORT, - ALPM_ERR_TRANS_TYPE, - ALPM_ERR_TRANS_NOT_LOCKED, - /* Packages */ - ALPM_ERR_PKG_NOT_FOUND, - ALPM_ERR_PKG_IGNORED, - ALPM_ERR_PKG_INVALID, - ALPM_ERR_PKG_INVALID_CHECKSUM, - ALPM_ERR_PKG_INVALID_SIG, - ALPM_ERR_PKG_MISSING_SIG, - ALPM_ERR_PKG_OPEN, - ALPM_ERR_PKG_CANT_REMOVE, - ALPM_ERR_PKG_INVALID_NAME, - ALPM_ERR_PKG_INVALID_ARCH, - ALPM_ERR_PKG_REPO_NOT_FOUND, - /* Signatures */ - ALPM_ERR_SIG_MISSING, - ALPM_ERR_SIG_INVALID, - /* Deltas */ - ALPM_ERR_DLT_INVALID, - ALPM_ERR_DLT_PATCHFAILED, - /* Dependencies */ - ALPM_ERR_UNSATISFIED_DEPS, - ALPM_ERR_CONFLICTING_DEPS, - ALPM_ERR_FILE_CONFLICTS, - /* Misc */ - ALPM_ERR_RETRIEVE, - ALPM_ERR_INVALID_REGEX, - /* External library errors */ - ALPM_ERR_LIBARCHIVE, - ALPM_ERR_LIBCURL, - ALPM_ERR_EXTERNAL_DOWNLOAD, - ALPM_ERR_GPGME -} alpm_errno_t; - -/** Returns the current error code from the handle. */ -alpm_errno_t alpm_errno(alpm_handle_t *handle); - -/** Returns the string corresponding to an error number. */ -const char *alpm_strerror(alpm_errno_t err); - -/* End of alpm_api_errors */ -/** @} */ - alpm_handle_t *alpm_initialize(const char *root, const char *dbpath, alpm_errno_t *err); int alpm_release(alpm_handle_t *handle); -- 1.9.0
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@jjacky.com> --- 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) +} 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. */ + 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. */ + 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 */ - 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, -- 1.9.0
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@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,
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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 66bb5f9..703b5f8 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, +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; + +/** + * 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 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; + /** 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..18a17df 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->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 */ - 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->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, -- 1.9.3
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags: callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf: testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~ apg
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Any suggestions of what we can do there?
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
apg
On 05/29/14 07:41, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Any suggestions of what we can do there?
I'm thinking turning the generic struct into an union of all the specific ones would fix it, something like: typedef struct _alpm_question_any_t { alpm_question_type_t type; int answer; } alpm_question_any_t; 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_t; Something similar to how XEvent works.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
apg
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(-) 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
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? 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
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
On 08/06/14 00:03, Olivier Brunel wrote:
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?
I looked at GdkEvent: http://www.gtk-server.org/gtk1/gdk/gdk-event-structures.html I would lean to union member access rather than typecasting, but not strongly. Any other opinions here? A
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> --- Now using union fields instead of typecasting. 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(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index a951162..db1e0cd 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -582,23 +582,119 @@ typedef union _alpm_event_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 88f5313..9cde1de 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -360,55 +360,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 = &question->install_ignorepkg; + 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 = &question->replace; + 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 = &question->conflict; + /* 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 = &question->remove_pkgs; 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++; @@ -419,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)); @@ -428,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 = &question->select_provider; + 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 = &question->corrupted; + 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 = &question->import_key; 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, -- 2.0.0
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this? Anyway, possible ways to fix this: 1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop This is crap because all clients that want to print a log message will need to wrap their calls like this. 2) Revert that change that made the logging an event. Unless someone comes up with something else, I'll do the revert soon. Allan
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this?
Anyway, possible ways to fix this:
1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop
This is crap because all clients that want to print a log message will need to wrap their calls like this.
2) Revert that change that made the logging an event.
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event). No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like. -j
Allan
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this?
Anyway, possible ways to fix this:
1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop
This is crap because all clients that want to print a log message will need to wrap their calls like this.
2) Revert that change that made the logging an event.
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end. Allan
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this?
Anyway, possible ways to fix this:
1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop
This is crap because all clients that want to print a log message will need to wrap their calls like this.
2) Revert that change that made the logging an event.
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter. All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user. Or did I not understand what you meant at all? -j
Allan
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> --- lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this?
Anyway, possible ways to fix this:
1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop
This is crap because all clients that want to print a log message will need to wrap their calls like this.
2) Revert that change that made the logging an event.
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend. I am in favour of reverting the log callback as the solution here. Allan
On 07/01/14 06:56, Allan McRae wrote:
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote:
On 05/24/14 at 03:49pm, 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@jjacky.com> > --- > lib/libalpm/alpm.h | 92 +++++++++++++++++++++++++++++++++++++++-- > 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, 222 insertions(+), 97 deletions(-)
The generic to specific struct casts cause Clang to fail when pacman is configured with --enable-warningflags:
callback.c:377:44: error: cast from 'alpm_question_t *' (aka 'struct _alpm_question_t *') to 'alpm_question_install_ignorepkg_t *' (aka 'struct _alpm_question_install_ignorepkg_t *') increases required alignment from 4 to 8 [-Werror,-Wcast-align] ...*q = (alpm_question_install_ignorepkg_t *) question; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
These are silenced.
The earlier changes to the event callback have the same issue and also give errors about using non-literal strings with vprintf:
testdb.c:53:11: error: format string is not a string literal [-Werror,-Wformat-nonliteral] vprintf(event->fmt, event->args); ^~~~~~~~~~
BAH! Why did gcc no see this?
Anyway, possible ways to fix this:
1) +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wformat-nonliteral" vprintf(event->fmt, event->args); +#pragma GCC diagnostic pop
This is crap because all clients that want to print a log message will need to wrap their calls like this.
2) Revert that change that made the logging an event.
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend.
I assume by prefixes you mean the front-end could add e.g. "warning:" before the message itself? Then sure, that part would be translated in the front-end, but I fail to see how getting the message as a string and not a format string & va_list would change/break anything? Either way, the front-end has no idea what the message is, nor does it care/do anything other than showing it to the user. (And pm_asprintf could simply be moved to util-common.c no?)
I am in favour of reverting the log callback as the solution here.
I feel it's a little sad there isn't a way to silence this false-positive warning, since this is really all it's about. I guess the log already had its own callback separated from events, although looking at the history it seems more to be because the log_cb was created first than for any actual reason, and "merging it" into events was a good idea IMO. (Of course, if there's ever another event with such a generated string/message, it'll have to be an event, and that'll make the log stand out even more for no good reason...) I just think making it an event was a good thing, but I don't feel very strongly about this, so I guess you could just revert to a separate log callback -- though I'm curious what's the main motivation for favoring this solution? Avoiding the malloc/free calls (A buffer could also be used/printed into then, leaving the malloc only for extra-long messages) ? -j
Allan
On 07/01/14 at 08:27pm, Olivier Brunel wrote:
On 07/01/14 06:56, Allan McRae wrote:
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote:
On 26/05/14 01:30, Andrew Gregory wrote: > The earlier changes to the event callback have the same issue and also > give errors about using non-literal strings with vprintf: > > testdb.c:53:11: error: format string is not a string literal > [-Werror,-Wformat-nonliteral] > vprintf(event->fmt, event->args); > ^~~~~~~~~~
BAH! Why did gcc no see this?
Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend.
I assume by prefixes you mean the front-end could add e.g. "warning:" before the message itself? Then sure, that part would be translated in the front-end, but I fail to see how getting the message as a string and not a format string & va_list would change/break anything?
Either way, the front-end has no idea what the message is, nor does it care/do anything other than showing it to the user.
(And pm_asprintf could simply be moved to util-common.c no?)
I am in favour of reverting the log callback as the solution here.
I feel it's a little sad there isn't a way to silence this false-positive warning, since this is really all it's about.
I guess the log already had its own callback separated from events, although looking at the history it seems more to be because the log_cb was created first than for any actual reason, and "merging it" into events was a good idea IMO.
(Of course, if there's ever another event with such a generated string/message, it'll have to be an event, and that'll make the log stand out even more for no good reason...)
I just think making it an event was a good thing, but I don't feel very strongly about this, so I guess you could just revert to a separate log callback -- though I'm curious what's the main motivation for favoring this solution? Avoiding the malloc/free calls (A buffer could also be used/printed into then, leaving the malloc only for extra-long messages) ?
Incorporating log messages into the event callback also silenced log messages when --print is used, including debug messages, which needs to be fixed. So, in order to keep it in the event callback you would have to: * create a special case for the log event in the callback when --print is used * malloc the formatted string in the back-end solely for the purpose of passing it to the front-end even if it will be ignored or could have been printed directly without the malloc * malloc a second copy of the string in the front-end whenever it's in the middle of a progress bar I don't think keeping log messages in the event callback is worth the hassle. apg
On 07/02/14 00:03, Andrew Gregory wrote:
On 07/01/14 at 08:27pm, Olivier Brunel wrote:
On 07/01/14 06:56, Allan McRae wrote:
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote:
On 06/24/14 08:14, Allan McRae wrote: > On 26/05/14 01:30, Andrew Gregory wrote: >> The earlier changes to the event callback have the same issue and also >> give errors about using non-literal strings with vprintf: >> >> testdb.c:53:11: error: format string is not a string literal >> [-Werror,-Wformat-nonliteral] >> vprintf(event->fmt, event->args); >> ^~~~~~~~~~ > > BAH! Why did gcc no see this? > > Unless someone comes up with something else, I'll do the revert soon.
I can't think of another way to silence the warning, but there's a way to "work around" it: instead of putting fmt & args inside alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() in alpm and send the resulting string in the event).
No more false warnings then (but a malloc/free for each log event). I can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend.
I assume by prefixes you mean the front-end could add e.g. "warning:" before the message itself? Then sure, that part would be translated in the front-end, but I fail to see how getting the message as a string and not a format string & va_list would change/break anything?
Either way, the front-end has no idea what the message is, nor does it care/do anything other than showing it to the user.
(And pm_asprintf could simply be moved to util-common.c no?)
I am in favour of reverting the log callback as the solution here.
I feel it's a little sad there isn't a way to silence this false-positive warning, since this is really all it's about.
I guess the log already had its own callback separated from events, although looking at the history it seems more to be because the log_cb was created first than for any actual reason, and "merging it" into events was a good idea IMO.
(Of course, if there's ever another event with such a generated string/message, it'll have to be an event, and that'll make the log stand out even more for no good reason...)
I just think making it an event was a good thing, but I don't feel very strongly about this, so I guess you could just revert to a separate log callback -- though I'm curious what's the main motivation for favoring this solution? Avoiding the malloc/free calls (A buffer could also be used/printed into then, leaving the malloc only for extra-long messages) ?
Incorporating log messages into the event callback also silenced log messages when --print is used, including debug messages, which needs
I don't understand. The commit that introduced log messages as event, that Allan will likely revert, shouldn't have changed anything in how/when said messages are printed, only how they are sent to the front-end (as in, which callback function is used). Also, I just ran `pacman --debug -S --print pacman` and I see debug messages just fine. So I'm not sure what you mean, could you tell me how to reproduce it?
to be fixed. So, in order to keep it in the event callback you would have to:
* create a special case for the log event in the callback when --print is used
As said earlier, I don't see why a special case is needed/what was broken here.
* malloc the formatted string in the back-end solely for the purpose of passing it to the front-end even if it will be ignored or could have been printed directly without the malloc
True (although it could be printed to a buffer, but yes)
* malloc a second copy of the string in the front-end whenever it's in the middle of a progress bar
True, but in that case pacman always mallocs its own copy of the message anyways, so nothing new here.
I don't think keeping log messages in the event callback is worth the hassle.
apg
On 07/02/14 at 01:02am, Olivier Brunel wrote:
On 07/02/14 00:03, Andrew Gregory wrote:
On 07/01/14 at 08:27pm, Olivier Brunel wrote:
On 07/01/14 06:56, Allan McRae wrote:
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote:
On 24/06/14 20:18, Olivier Brunel wrote: > On 06/24/14 08:14, Allan McRae wrote: >> On 26/05/14 01:30, Andrew Gregory wrote: >>> The earlier changes to the event callback have the same issue and also >>> give errors about using non-literal strings with vprintf: >>> >>> testdb.c:53:11: error: format string is not a string literal >>> [-Werror,-Wformat-nonliteral] >>> vprintf(event->fmt, event->args); >>> ^~~~~~~~~~ >> >> BAH! Why did gcc no see this? >> >> Unless someone comes up with something else, I'll do the revert soon. > > I can't think of another way to silence the warning, but there's a way > to "work around" it: instead of putting fmt & args inside > alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() > in alpm and send the resulting string in the event). > > No more false warnings then (but a malloc/free for each log event). I > can send a patch if you'd like.
Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend.
I assume by prefixes you mean the front-end could add e.g. "warning:" before the message itself? Then sure, that part would be translated in the front-end, but I fail to see how getting the message as a string and not a format string & va_list would change/break anything?
Either way, the front-end has no idea what the message is, nor does it care/do anything other than showing it to the user.
(And pm_asprintf could simply be moved to util-common.c no?)
I am in favour of reverting the log callback as the solution here.
I feel it's a little sad there isn't a way to silence this false-positive warning, since this is really all it's about.
I guess the log already had its own callback separated from events, although looking at the history it seems more to be because the log_cb was created first than for any actual reason, and "merging it" into events was a good idea IMO.
(Of course, if there's ever another event with such a generated string/message, it'll have to be an event, and that'll make the log stand out even more for no good reason...)
I just think making it an event was a good thing, but I don't feel very strongly about this, so I guess you could just revert to a separate log callback -- though I'm curious what's the main motivation for favoring this solution? Avoiding the malloc/free calls (A buffer could also be used/printed into then, leaving the malloc only for extra-long messages) ?
Incorporating log messages into the event callback also silenced log messages when --print is used, including debug messages, which needs
I don't understand. The commit that introduced log messages as event, that Allan will likely revert, shouldn't have changed anything in how/when said messages are printed, only how they are sent to the front-end (as in, which callback function is used).
Also, I just ran `pacman --debug -S --print pacman` and I see debug messages just fine. So I'm not sure what you mean, could you tell me how to reproduce it?
void cb_event(alpm_event_t *event) { if(config->print) { return; } The only messages you see are from pacman not alpm. apg
On 07/02/14 01:10, Andrew Gregory wrote:
On 07/02/14 at 01:02am, Olivier Brunel wrote:
On 07/02/14 00:03, Andrew Gregory wrote:
On 07/01/14 at 08:27pm, Olivier Brunel wrote:
On 07/01/14 06:56, Allan McRae wrote:
On 29/06/14 21:35, Olivier Brunel wrote:
On 06/29/14 03:50, Allan McRae wrote: > On 24/06/14 20:18, Olivier Brunel wrote: >> On 06/24/14 08:14, Allan McRae wrote: >>> On 26/05/14 01:30, Andrew Gregory wrote: >>>> The earlier changes to the event callback have the same issue and also >>>> give errors about using non-literal strings with vprintf: >>>> >>>> testdb.c:53:11: error: format string is not a string literal >>>> [-Werror,-Wformat-nonliteral] >>>> vprintf(event->fmt, event->args); >>>> ^~~~~~~~~~ >>> >>> BAH! Why did gcc no see this? >>> >>> Unless someone comes up with something else, I'll do the revert soon. >> >> I can't think of another way to silence the warning, but there's a way >> to "work around" it: instead of putting fmt & args inside >> alpm_event_log_t, we put the resulting string (i.e. call pm_asprintf() >> in alpm and send the resulting string in the event). >> >> No more false warnings then (but a malloc/free for each log event). I >> can send a patch if you'd like. > > Can not do that. Translations of messages happen in the front-end.
um, no? Translations obviously happen in ALPM, the call to gettext happens before calling _alpm_log(), so by then whether we send the message to the front-end as a pointer to the full string, or a pointer to a format string and a va_list, doesn't really matter.
All the front-end does is decide whether or not (e.g. based on level), and how, to show the message to the user.
Or did I not understand what you meant at all?
Some parts are, some are not (the prefixes). Also, pm_asprintf is not available in the backend.
I assume by prefixes you mean the front-end could add e.g. "warning:" before the message itself? Then sure, that part would be translated in the front-end, but I fail to see how getting the message as a string and not a format string & va_list would change/break anything?
Either way, the front-end has no idea what the message is, nor does it care/do anything other than showing it to the user.
(And pm_asprintf could simply be moved to util-common.c no?)
I am in favour of reverting the log callback as the solution here.
I feel it's a little sad there isn't a way to silence this false-positive warning, since this is really all it's about.
I guess the log already had its own callback separated from events, although looking at the history it seems more to be because the log_cb was created first than for any actual reason, and "merging it" into events was a good idea IMO.
(Of course, if there's ever another event with such a generated string/message, it'll have to be an event, and that'll make the log stand out even more for no good reason...)
I just think making it an event was a good thing, but I don't feel very strongly about this, so I guess you could just revert to a separate log callback -- though I'm curious what's the main motivation for favoring this solution? Avoiding the malloc/free calls (A buffer could also be used/printed into then, leaving the malloc only for extra-long messages) ?
Incorporating log messages into the event callback also silenced log messages when --print is used, including debug messages, which needs
I don't understand. The commit that introduced log messages as event, that Allan will likely revert, shouldn't have changed anything in how/when said messages are printed, only how they are sent to the front-end (as in, which callback function is used).
Also, I just ran `pacman --debug -S --print pacman` and I see debug messages just fine. So I'm not sure what you mean, could you tell me how to reproduce it?
void cb_event(alpm_event_t *event) { if(config->print) { return; }
The only messages you see are from pacman not alpm.
Oh right, got it; thanks. Yeah the case of ALPM_EVENT_LOG should have been handled before that test, indeed.
apg
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Olivier Brunel