2016-01-02 19:21 GMT+01:00 Olivier Brunel :

As reported by Rikard Falkeborn[1] using event-specific struct and then typecasting to the generic alpm_event_t could possibly lead to alignment issue, so we now always use alpm_event_t instead.

[1] https://lists.archlinux.org/pipermail/pacman-dev/2015-December/020709.html

Signed-off-by: Olivier Brunel --- lib/libalpm/add.c | 36 ++++++++++++++++++------------------ lib/libalpm/be_sync.c | 4 ++-- lib/libalpm/handle.h | 2 +- lib/libalpm/hook.c | 14 +++++++------- lib/libalpm/remove.c | 20 ++++++++++---------- lib/libalpm/sync.c | 8 ++++---- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 4 ++-- 8 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 63eda49..4b76bc7 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -315,12 +315,12 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, }

if(notouch) { - alpm_event_pacnew_created_t event = { + alpm_event_t event = { .type = ALPM_EVENT_PACNEW_CREATED, - .from_noupgrade = 1, - .oldpkg = oldpkg, - .newpkg = newpkg, - .file = filename + .pacnew_created.from_noupgrade = 1, + .pacnew_created.oldpkg = oldpkg, + .pacnew_created.newpkg = newpkg, + .pacnew_created.file = filename }; /* "remove" the .pacnew suffix */ filename[filename_len] = '\0'; @@ -366,12 +366,12 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, } else { /* none of the three files matched another, leave the unpacked * file alongside the local file */ - alpm_event_pacnew_created_t event = { + alpm_event_t event = { .type = ALPM_EVENT_PACNEW_CREATED, - .from_noupgrade = 0, - .oldpkg = oldpkg, - .newpkg = newpkg, - .file = origfile + .pacnew_created.from_noupgrade = 0, + .pacnew_created.oldpkg = oldpkg, + .pacnew_created.newpkg = newpkg, + .pacnew_created.file = origfile }; _alpm_log(handle, ALPM_LOG_DEBUG, "action: keeping current file and installing" @@ -398,7 +398,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, alpm_db_t *db = handle->db_local; alpm_trans_t *trans = handle->trans; alpm_progress_t progress = ALPM_PROGRESS_ADD_START; - alpm_event_package_operation_t event; + alpm_event_t event; const char *log_msg = "adding"; const char *pkgfile;

@@ -411,15 +411,15 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, if(cmp < 0) { log_msg = "downgrading"; progress = ALPM_PROGRESS_DOWNGRADE_START; - event.operation = ALPM_PACKAGE_DOWNGRADE; + event.package_operation.operation = ALPM_PACKAGE_DOWNGRADE; } else if(cmp == 0) { log_msg = "reinstalling"; progress = ALPM_PROGRESS_REINSTALL_START; - event.operation = ALPM_PACKAGE_REINSTALL; + event.package_operation.operation = ALPM_PACKAGE_REINSTALL; } else { log_msg = "upgrading"; progress = ALPM_PROGRESS_UPGRADE_START; - event.operation = ALPM_PACKAGE_UPGRADE; + event.package_operation.operation = ALPM_PACKAGE_UPGRADE; } is_upgrade = 1;

@@ -432,12 +432,12 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, /* copy over the install reason */ newpkg->reason = alpm_pkg_get_reason(local); } else { - event.operation = ALPM_PACKAGE_INSTALL; + event.package_operation.operation = ALPM_PACKAGE_INSTALL; }

event.type = ALPM_EVENT_PACKAGE_OPERATION_START; - event.oldpkg = oldpkg; - event.newpkg = newpkg; + event.package_operation.oldpkg = oldpkg; + event.package_operation.newpkg = newpkg; EVENT(handle, &event);

pkgfile = newpkg->origin_data.file; @@ -589,7 +589,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg,

PROGRESS(handle, progress, newpkg->name, 100, pkg_count, pkg_current);

- switch(event.operation) { + switch(event.package_operation.operation) { case ALPM_PACKAGE_INSTALL: alpm_logaction(handle, ALPM_CALLER_PREFIX, "installed %s (%s)\n", newpkg->name, newpkg->version); diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 200e381..2ad01f5 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -90,9 +90,9 @@ static int sync_db_validate(alpm_db_t *db)

/* we can skip any validation if the database doesn't exist */ if(_alpm_access(db->handle, NULL, dbpath, R_OK) != 0 && errno == ENOENT) { - alpm_event_database_missing_t event = { + alpm_event_t event = { .type = ALPM_EVENT_DATABASE_MISSING, - .dbname = db->treename + .database_missing.dbname = db->treename }; db->status &= ~DB_STATUS_EXISTS; db->status |= DB_STATUS_MISSING; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index e252fbf..2682fd4 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -34,7 +34,7 @@ #define EVENT(h, e) \ do { \ if((h)->eventcb) { \ - (h)->eventcb((alpm_event_t *) (e)); \ + (h)->eventcb(e); \ } \ } while(0) #define QUESTION(h, q) \ diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index b5ed17d..e9b50e8 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -614,8 +614,8 @@ static int _alpm_hook_run_hook(alpm_handle_t *handle, struct _alpm_hook_t *hook)

int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) { - alpm_event_hook_t event = { .when = when }; - alpm_event_hook_run_t hook_event; + alpm_event_t event = { .hook.when = when }; + alpm_event_t hook_event; alpm_list_t *i, *hooks = NULL, *hooks_triggered = NULL; const char *suffix = ".hook"; size_t suflen = strlen(suffix), triggered = 0; @@ -731,16 +731,16 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) event.type = ALPM_EVENT_HOOK_START; EVENT(handle, &event);

- hook_event.position = 1; - hook_event.total = triggered; + hook_event.hook_run.position = 1; + hook_event.hook_run.total = triggered;

- for(i = hooks_triggered; i; i = i->next, hook_event.position++) { + for(i = hooks_triggered; i; i = i->next, hook_event.hook_run.position++) { struct _alpm_hook_t *hook = i->data; _alpm_log(handle, ALPM_LOG_DEBUG, "running hook %s\n", hook->name);

hook_event.type = ALPM_EVENT_HOOK_RUN_START; - hook_event.name = hook->name; - hook_event.desc = hook->desc; + hook_event.hook_run.name = hook->name; + hook_event.hook_run.desc = hook->desc; EVENT(handle, &hook_event);

if(_alpm_hook_run_hook(handle, hook) != 0 && hook->abort_on_fail) { diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 2fb7993..c7473e4 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -181,10 +181,10 @@ static void remove_notify_needed_optdepends(alpm_handle_t *handle, alpm_list_t * for(j = optdeps; j; j = alpm_list_next(j)) { alpm_depend_t *optdep = j->data; if(alpm_pkg_find(lp, optdep->name)) { - alpm_event_optdep_removal_t event = { + alpm_event_t event = { .type = ALPM_EVENT_OPTDEP_REMOVAL, - .pkg = pkg, - .optdep = optdep + .optdep_removal.pkg = pkg, + .optdep_removal.optdep = optdep }; EVENT(handle, &event); } @@ -506,10 +506,10 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, int cmp = filehash ? strcmp(filehash, backup->hash) : 0; FREE(filehash); if(cmp != 0) { - alpm_event_pacsave_created_t event = { + alpm_event_t event = { .type = ALPM_EVENT_PACSAVE_CREATED, - .oldpkg = oldpkg, - .file = file + .pacsave_created.oldpkg = oldpkg, + .pacsave_created.file = file }; char *newpath; size_t len = strlen(file) + 8 + 1; @@ -658,11 +658,11 @@ int _alpm_remove_single_package(alpm_handle_t *handle, { const char *pkgname = oldpkg->name; const char *pkgver = oldpkg->version; - alpm_event_package_operation_t event = { + alpm_event_t event = { .type = ALPM_EVENT_PACKAGE_OPERATION_START, - .operation = ALPM_PACKAGE_REMOVE, - .oldpkg = oldpkg, - .newpkg = NULL + .package_operation.operation = ALPM_PACKAGE_REMOVE, + .package_operation.oldpkg = oldpkg, + .package_operation.newpkg = NULL };

if(newpkg) { diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index c5607bc..27528d5 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -704,7 +704,7 @@ static int apply_deltas(alpm_handle_t *handle) int ret = 0; const char *cachedir = _alpm_filecache_setup(handle); alpm_trans_t *trans = handle->trans; - alpm_event_delta_patch_t event; + alpm_event_t event;

for(i = trans->add; i; i = i->next) { alpm_pkg_t *spkg = i->data; @@ -754,7 +754,7 @@ static int apply_deltas(alpm_handle_t *handle) _alpm_log(handle, ALPM_LOG_DEBUG, "command: %s\n", command);

event.type = ALPM_EVENT_DELTA_PATCH_START; - event.delta = d; + event.delta_patch.delta = d; EVENT(handle, &event);

int retval = system(command); @@ -917,9 +917,9 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t static int download_single_file(alpm_handle_t *handle, struct dload_payload *payload, const char *cachedir) { - alpm_event_pkgdownload_t event = { + alpm_event_t event = { .type = ALPM_EVENT_PKGDOWNLOAD_START, - .file = payload->remote_name + .pkgdownload.file = payload->remote_name }; const alpm_list_t *server;

diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 5b52049..bfb3115 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -160,7 +160,7 @@ int SYMEXPORT alpm_trans_prepare(alpm_handle_t *handle, alpm_list_t **data) int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) { alpm_trans_t *trans; - alpm_event_any_t event; + alpm_event_t event;

/* Sanity checks */ CHECK_HANDLE(handle, return -1); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index 3824d13..6d14da7 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -489,9 +489,9 @@ static int _alpm_chroot_write_to_child(alpm_handle_t *handle, int fd,

static void _alpm_chroot_process_output(alpm_handle_t *handle, const char *line) { - alpm_event_scriptlet_info_t event = { + alpm_event_t event = { .type = ALPM_EVENT_SCRIPTLET_INFO, - .line = line + .scriptlet_info.line = line }; alpm_logaction(handle, "ALPM-SCRIPTLET", "%s", line); EVENT(handle, &event); -- 2.6.4

Hello, Clang is still not happy, same types of warnings in all the files changed (this output from just running ./configure CC=clang, i.e., no extra warningflags enabled): CC libalpm_la-add.lo add.c:320:5: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides] .pacnew_created.from_noupgrade = 1, ~^~~~~~~~~~~~~~ add.c:319:12: note: previous initialization is here .type = ALPM_EVENT_PACNEW_CREATED, ^~~~~~~~~~~~~~~~~~~~~~~~~ add.c:371:6: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides] .pacnew_created.from_noupgrade = 0, ~^~~~~~~~~~~~~~ add.c:370:13: note: previous initialization is here .type = ALPM_EVENT_PACNEW_CREATED, ^~~~~~~~~~~~~~~~~~~~~~~~~ 2 warnings generated. Changing .type = ALPM_EVENT_PACNEW_CREATED, to .pacnew_created.type = ALPM_EVENT_PACNEW_CREATED, produces no warnings. /Rikard

On 04/01/16 19:58, Andrew Gregory wrote:

On 01/02/16 at 10:14pm, Olivier Brunel wrote:

...

As reported by Rikard Falkeborn[1] using event-specific struct and then typecasting to the generic alpm_event_t could possibly lead to alignment issue, so we now always use alpm_event_t instead.

[1] https://lists.archlinux.org/pipermail/pacman-dev/2015-December/020709.html

Signed-off-by: Olivier Brunel --- Now always using the union member, to avoid initializer warnings (thanks Rikard) and better consistency. Hopefully this time it's all good.

lib/libalpm/add.c | 44 ++++++++++++++++++++++---------------------- lib/libalpm/be_sync.c | 6 +++--- lib/libalpm/handle.h | 2 +- lib/libalpm/hook.c | 22 +++++++++++----------- lib/libalpm/remove.c | 28 ++++++++++++++-------------- lib/libalpm/sync.c | 14 +++++++------- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 6 +++--- 8 files changed, 62 insertions(+), 62 deletions(-)

Is there any reason not to just silence the warning by casting the event to (void*)?

Something like this? diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index 49a5dae..ed79fdd 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -729,7 +729,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) if(hooks_triggered != NULL) { event.type = ALPM_EVENT_HOOK_START; - EVENT(handle, &event); + EVENT(handle, (void *)&event); hook_event.position = 1; hook_event.total = triggered; @@ -758,7 +758,7 @@ int _alpm_hook_run(alpm_handle_t *handle, alpm_hook_when_t when) alpm_list_free(hooks_triggered); event.type = ALPM_EVENT_HOOK_DONE; - EVENT(handle, &event); + EVENT(handle, (void *)&event); } cleanup: diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index 7e6d7bc..239d6a1 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -199,7 +199,7 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction started\n"); event.type = ALPM_EVENT_TRANSACTION_START; - EVENT(handle, &event); + EVENT(handle, (void *)&event); if(trans->add == NULL) { if(_alpm_remove_packages(handle, 1) == -1) { @@ -223,7 +223,7 @@ int SYMEXPORT alpm_trans_commit(alpm_handle_t *handle, alpm_list_t **data) alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction interrupted\n"); } else { event.type = ALPM_EVENT_TRANSACTION_DONE; - EVENT(handle, &event); + EVENT(handle, (void *)&event); alpm_logaction(handle, ALPM_CALLER_PREFIX, "transaction completed\n"); _alpm_hook_run(handle, ALPM_HOOK_POST_TRANSACTION); }