On 11/15/13 12:28, Allan McRae wrote: <snip>
Now I am not sure about the oldpkg/newpkg in an array passing. I think just passing pkgname is a reasonable compromise. I guess the frontend then can grab the old package version easily enough.
As I said earlier, I'd like to have oldpkg for pacnew, but there's another reason to send both oldpkg & newpkg (as well as filename) to the callback: a pacnew file can also be created when a file is in NoUpgrade, in which case it should be oldpkg == NULL that indicates why the pacnew was created. Now, about Andrew's suggestion to get rid of the (void*, void*) callback, replacing it with a pointer to a struct, I've been thinking about this, and of all the ideas mentionned here's the one I like best: using an alpm_event_t struct that is passed to the callback as sole argument. This struct contains only one member, giving the type of event we're dealing with. Many events do not actually have any arguments, so that's that for those. For events where there are arguments, it is then possible to recast to the "correct"/event-specific struct, where the corresponding arguments can be found. This is somewhat like what I suggested earlier, only without an union (because I'm not sure we're gaining much using one). All that's needed is that every event-specific struct has the type as its first member. For example: typedef struct { alpm_event_type_t type; } alpm_event_t; typedef struct { alpm_event_type_t type; alpm_pkg_t oldpkg; alpm_pkg_t newpkg; } alpm_event_upgrade_t; void callback (alpm_event_t *_event) { if (event->type == ALPM_EVENT_UPGRADE_START) { alpm_event_upgrade_t *event = (alpm_event_upgrade_t *) _event; printf ("upgrading %s (%s -> %s)\n", alpm_pkg_get_name (event->newpkg), alpm_pkg_get_version (event->oldpkg), alpm_pkg_get_version (event->newpkg)); } } This allows to have (as many as needed) even-specific arguments and give everything proper names and types. I feel it's also simpler to use and document than having only one struct with all possible fields, and needing to know which ones are used/available on what event. Now, a lot of events work in pair, with a START & a DONE one. I think it would be fine to have one single struct for both of those, with the (currently) one exception of ALPM_EVENT_DELTA_PATCH_START where START has a couple of arguments (pkg_filename, patch_filename) whereas DONE has none. But in this case I think it's fine to simply document that the alpm_event_delta_patch_t struct only applied to START, and DONE has only the alpm_event_t struct. However, for events that take the same arguments but are unrelated, e.g. the pairs for UPGRADE, DOWNGRADE & REINSTALL (which all take oldpkg & newpkg) I think we should have one typedef for each pair of events, as it makes things clearer (the goal being that every struct is event-specific, not based on what arguments it (currently) takes) and will be much better in case one of those shall take more arguments in the future. Going over things, here's what I'd suggest: typedef struct { alpm_event_type_t type; } alpm_event_t; ALPM_EVENT_CHECKDEPS_START ALPM_EVENT_CHECKDEPS_DONE ALPM_EVENT_FILECONFLICTS_START ALPM_EVENT_FILECONFLICTS_DONE ALPM_EVENT_RESOLVEDEPS_START ALPM_EVENT_RESOLVEDEPS_DONE ALPM_EVENT_INTERCONFLICTS_START ALPM_EVENT_INTERCONFLICTS_DONE ALPM_EVENT_INTEGRITY_START ALPM_EVENT_INTEGRITY_DONE ALPM_EVENT_LOAD_START ALPM_EVENT_LOAD_DONE ALPM_EVENT_DELTA_INTEGRITY_START ALPM_EVENT_DELTA_INTEGRITY_DONE ALPM_EVENT_DELTA_PATCHES_START ALPM_EVENT_DELTA_PATCHES_DONE ALPM_EVENT_DELTA_PATCH_DONE ALPM_EVENT_DELTA_PATCH_FAILED ALPM_EVENT_RETRIEVE_START ALPM_EVENT_DISKSPACE_START ALPM_EVENT_DISKSPACE_DONE ALPM_EVENT_KEYRING_START ALPM_EVENT_KEYRING_DONE ALPM_EVENT_KEY_DOWNLOAD_START ALPM_EVENT_KEY_DOWNLOAD_DONE typedef struct { alpm_event_type_t type; alpm_pkg_t newpkg; } alpm_event_add_t; ALPM_EVENT_ADD_START ALPM_EVENT_ADD_DONE typedef struct { alpm_event_type_t type; alpm_pkg_t oldpkg; } alpm_event_remove_t; ALPM_EVENT_REMOVE_START ALPM_EVENT_REMOVE_DONE typedef struct { alpm_event_type_t type; alpm_pkg_t oldpkg; alpm_pkg_t newpkg; } alpm_event_upgrade_t; ALPM_EVENT_UPGRADE_START ALPM_EVENT_UPGRADE_DONE typedef struct { alpm_event_type_t type; alpm_pkg_t oldpkg; alpm_pkg_t newpkg; } alpm_event_downgrade_t; ALPM_EVENT_DOWNGRADE_START ALPM_EVENT_DOWNGRADE_DONE typedef struct { alpm_event_type_t type; alpm_pkg_t oldpkg; alpm_pkg_t newpkg; } alpm_event_reinstall_t; ALPM_EVENT_REINSTALL_START ALPM_EVENT_REINSTALL_DONE Note that really the 3 typedefs above could resolve to the same _alpm_event_update_t struct. typedef struct { alpm_event_type_t type; alpm_pkg_t pkg; alpm_depend_t optdep; } alpm_event_optdep_t; ALPM_EVENT_OPTDEP_REQUIRED Might be me, but isn't that one "oddly" named. It is emitted when an optdep is being removed, so wouldn't ALPM_EVENT_OPTDEP_REMOVED be more appropriate? Since we're talking optdep here, there's really no requirement. Just because a package that is optdep for another one is removed doesn't mean it was even used as optdep, much less is required, no? typedef struct { alpm_event_type_t type; const char *pkg_filename; const char *patch_filename; } alpm_event_delta_patch_t; ALPM_EVENT_DELTA_PATCH_START typedef struct { alpm_event_type_t type; const char *line; } alpm_event_scriptlet_t; ALPM_EVENT_SCRIPTLET_INFO typedef struct { alpm_event_type_t type; const char *dbname; } alpm_event_database_missing_t; ALPM_EVENT_DATABASE_MISSING typedef struct { alpm_event_type_t type; alpm_pkg_t *oldpkg; /* NULL when creation from NoUpgrade */ alpm_pkg_t *newpkg; const char *oldfile; } alpm_event_pacnew_t; ALPM_EVENT_PACNEW_CREATED typedef struct { alpm_event_type_t type; alpm_pkg_t *newpkg; const char *oldfile; } alpm_event_pacorig_t; ALPM_EVENT_PACORIG_CREATED typedef struct { alpm_event_type_t type; alpm_pkg_t *oldpkg; const char *oldfile; } alpm_event_pacsave_t; ALPM_EVENT_PACSAVE_CREATED And new events would need to be created, to get rid of the log_cb: typedef struct { alpm_event_type_t type; const char *message; } alpm_event_warning_t; ALPM_EVENT_WARNING typedef struct { alpm_event_type_t type; const char *message; } alpm_event_error_t; ALPM_EVENT_ERROR typedef struct { alpm_event_type_t type; const char *message; } alpm_event_debug_t; ALPM_EVENT_DEBUG typedef struct { alpm_event_type_t type; const char *message; } alpm_event_function_t; ALPM_EVENT_FUNCTION As before, all 4 typedef could resolve to the same _alpm_event_msg_t struct. (Unless you'd rather have one single event, and an level to indicate whether it is a warning/error/etc ?) I think all debug messages & errors shall remain as such. For warnings though, we might want to turn some into actual events. In addition to the creation of pac* files, I'd suggest the following: typedef struct { alpm_event_type_t type; const char *url; } alpm_event_download_t; ALPM_EVENT_DOWNLOAD_DONE ALPM_EVENT_DOWNLOAD_FAILED There's a warning in alpm_fetch_pkgurl() in case of failure; I think it might be good to turn that into an event, but also add such events in download_files() (in sync.c) instead of, also, just a generic warning on failure ("failed to retrieve some files"). Also, ALPM_EVENT_RETRIEVE_START doesn't have a matching DONE and I think it'd be a good idea to add one as well (so a frontend can know when the downloading part is done) (Maybe a FAILED too?). Thoughts? -j