[pacman-dev] [PATCH] Refactor the trans init and release code.
Dan McGee
dpmcgee at gmail.com
Sat Apr 26 10:51:35 EDT 2008
On Sat, Apr 26, 2008 at 8:37 AM, Chantry Xavier <shiningxc at gmail.com> wrote:
> The calls to alpm_trans_init and alpm_trans_release (+ error checking) were
> duplicated between remove.c, sync.c and upgrade.c
> This patch introduces trans_init and trans_release functions in util.c to
> have this code just once.
>
> So instead of having to do the same change 3 times for fixing FS#10273, I
> just had to do it once (so I did it too :))
Much better, thanks for the cleanup. Comments inline.
> Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
> ---
> src/pacman/remove.c | 35 ++++++++---------------------------
> src/pacman/sync.c | 39 ++++++++-------------------------------
> src/pacman/upgrade.c | 34 +++++++---------------------------
> src/pacman/util.c | 28 ++++++++++++++++++++++++++++
> src/pacman/util.h | 2 ++
> 5 files changed, 53 insertions(+), 85 deletions(-)
>
> diff --git a/src/pacman/remove.c b/src/pacman/remove.c
> index 2fb69d3..a2209ac 100644
> --- a/src/pacman/remove.c
> +++ b/src/pacman/remove.c
> @@ -29,24 +29,10 @@
> /* pacman */
> #include "pacman.h"
> #include "util.h"
> -#include "callback.h"
> #include "conf.h"
>
> extern pmdb_t *db_local;
>
> -/* Free the current transaction and print an error if unsuccessful */
> -static int remove_cleanup(void)
> -{
> - int ret = alpm_trans_release();
> - if(ret != 0) {
> - pm_printf(PM_LOG_ERROR, _("failed to release transaction (%s)\n"),
> - alpm_strerrorlast());
> - ret = 1;
> - }
> -
> - return(ret);
> -}
> -
> /**
> * @brief Remove a specified list of packages.
> *
> @@ -90,14 +76,7 @@ int pacman_remove(alpm_list_t *targets)
> }
>
> /* Step 1: create a new transaction */
> - if(alpm_trans_init(PM_TRANS_TYPE_REMOVE, config->flags,
> - cb_trans_evt, cb_trans_conv, cb_trans_progress) == -1) {
> - fprintf(stderr, _("error: failed to init transaction (%s)\n"),
> - alpm_strerrorlast());
> - if(pm_errno == PM_ERR_HANDLE_LOCK) {
> - printf(_(" if you're sure a package manager is not already\n"
> - " running, you can remove %s.\n"), alpm_option_get_lockfile());
> - }
> + if(trans_init(PM_TRANS_TYPE_REMOVE, config->flags) == -1) {
> FREELIST(finaltargs);
> return(1);
> }
> @@ -109,7 +88,7 @@ int pacman_remove(alpm_list_t *targets)
> if(alpm_trans_addtarget(targ) == -1) {
> fprintf(stderr, _("error: '%s': %s\n"),
> targ, alpm_strerrorlast());
> - remove_cleanup();
> + trans_release();
> FREELIST(finaltargs);
> return(1);
> }
> @@ -134,7 +113,7 @@ int pacman_remove(alpm_list_t *targets)
> default:
> break;
> }
> - remove_cleanup();
> + trans_release();
We never check the return value of trans_release(), but I guess that
is OK because we are returning an error condition anyway? This happens
about 50 times.
> FREELIST(finaltargs);
> return(1);
> }
> @@ -154,7 +133,7 @@ int pacman_remove(alpm_list_t *targets)
> FREELIST(lst);
> /* get confirmation */
> if(yesno(1, _("\nDo you want to remove these packages?")) == 0) {
> - remove_cleanup();
> + trans_release();
> FREELIST(finaltargs);
> return(1);
> }
> @@ -165,13 +144,15 @@ int pacman_remove(alpm_list_t *targets)
> if(alpm_trans_commit(NULL) == -1) {
> fprintf(stderr, _("error: failed to commit transaction (%s)\n"),
> alpm_strerrorlast());
> - remove_cleanup();
> + trans_release();
> FREELIST(finaltargs);
> return(1);
> }
>
> /* Step 4: release transaction resources */
> - retval = remove_cleanup();
> + if(trans_release() == -1) {
> + retval = 1;
> + }
Ahh, so we do check it here. Good.
> FREELIST(finaltargs);
> return(retval);
> }
> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
> index c074746..77b66da 100644
> --- a/src/pacman/sync.c
> +++ b/src/pacman/sync.c
> @@ -34,7 +34,6 @@
> #include "pacman.h"
> #include "util.h"
> #include "package.h"
> -#include "callback.h"
Wow, even cleaning out the headers. Nice work.
> #include "conf.h"
>
> extern pmdb_t *db_local;
> @@ -235,34 +234,12 @@ static int sync_cleancache(int level)
> return(0);
> }
>
> -static int sync_trans_init(pmtransflag_t flags) {
> - if(alpm_trans_init(PM_TRANS_TYPE_SYNC, flags, cb_trans_evt,
> - cb_trans_conv, cb_trans_progress) == -1) {
> - fprintf(stderr, _("error: failed to init transaction (%s)\n"),
> - alpm_strerrorlast());
> - if(pm_errno == PM_ERR_HANDLE_LOCK) {
> - printf(_(" if you're sure a package manager is not already\n"
> - " running, you can remove %s.\n"), alpm_option_get_lockfile());
> - }
> - return(-1);
> - }
> - return(0);
> -}
> -
> -static int sync_trans_release() {
> - if(alpm_trans_release() == -1) {
> - fprintf(stderr, _("error: failed to release transaction (%s)\n"),
> - alpm_strerrorlast());
> - return(-1);
> - }
> - return(0);
> -}
> static int sync_synctree(int level, alpm_list_t *syncs)
> {
> alpm_list_t *i;
> int success = 0, ret;
>
> - if(sync_trans_init(0) == -1) {
> + if(trans_init(PM_TRANS_TYPE_SYNC, 0) == -1) {
> return(0);
> }
>
> @@ -281,7 +258,7 @@ static int sync_synctree(int level, alpm_list_t *syncs)
> }
> }
>
> - if(sync_trans_release() == -1) {
> + if(trans_release() == -1) {
> return(0);
> }
> /* We should always succeed if at least one DB was upgraded - we may possibly
> @@ -552,7 +529,7 @@ static int sync_trans(alpm_list_t *targets)
> alpm_list_t *sync_dbs = alpm_option_get_syncdbs();
>
> /* Step 1: create a new transaction... */
> - if(sync_trans_init(config->flags) == -1) {
> + if(trans_init(PM_TRANS_TYPE_SYNC, config->flags) == -1) {
> return(1);
> }
>
> @@ -583,10 +560,10 @@ static int sync_trans(alpm_list_t *targets)
> printf(_(":: pacman has detected a newer version of itself.\n"));
> if(yesno(1, _(":: Do you want to cancel the current operation\n"
> ":: and install the new pacman version now?"))) {
> - if(sync_trans_release() == -1) {
> + if(trans_release() == -1) {
> return(1);
> }
> - if(sync_trans_init(0) == -1) {
> + if(trans_init(PM_TRANS_TYPE_SYNC, 0) == -1) {
> return(1);
> }
> if(alpm_trans_addtarget("pacman") == -1) {
> @@ -788,7 +765,7 @@ cleanup:
> if(data) {
> FREELIST(data);
> }
> - if(sync_trans_release() == -1) {
> + if(trans_release() == -1) {
> retval = 1;
> }
>
> @@ -803,14 +780,14 @@ int pacman_sync(alpm_list_t *targets)
> if(config->op_s_clean) {
> int ret = 0;
>
> - if(sync_trans_init(0) == -1) {
> + if(trans_init(PM_TRANS_TYPE_SYNC, 0) == -1) {
> return(1);
> }
>
> ret += sync_cleancache(config->op_s_clean);
> ret += sync_cleandb_all();
>
> - if(sync_trans_release() == -1) {
> + if(trans_release() == -1) {
> ret++;
> }
>
> diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c
> index e562651..589970e 100644
> --- a/src/pacman/upgrade.c
> +++ b/src/pacman/upgrade.c
> @@ -28,23 +28,9 @@
>
> /* pacman */
> #include "pacman.h"
> -#include "callback.h"
> #include "conf.h"
> #include "util.h"
>
> -/* Free the current transaction and print an error if unsuccessful */
> -static int upgrade_cleanup(void)
> -{
> - int ret = alpm_trans_release();
> - if(ret != 0) {
> - pm_printf(PM_LOG_ERROR, _("failed to release transaction (%s)\n"),
> - alpm_strerrorlast());
> - ret = 1;
> - }
> -
> - return(ret);
> -}
> -
> /**
> * @brief Upgrade a specified list of packages.
> *
> @@ -78,15 +64,7 @@ int pacman_upgrade(alpm_list_t *targets)
> }
>
> /* Step 1: create a new transaction */
> - if(alpm_trans_init(transtype, config->flags, cb_trans_evt,
> - cb_trans_conv, cb_trans_progress) == -1) {
> - /* TODO: error messages should be in the front end, not the back */
> - fprintf(stderr, _("error: %s\n"), alpm_strerrorlast());
> - if(pm_errno == PM_ERR_HANDLE_LOCK) {
> - /* TODO this and the 2 other places should probably be on stderr */
> - printf(_(" if you're sure a package manager is not already\n"
> - " running, you can remove %s.\n"), alpm_option_get_lockfile());
> - }
> + if(trans_init(transtype, config->flags) == -1) {
> return(1);
> }
>
> @@ -97,7 +75,7 @@ int pacman_upgrade(alpm_list_t *targets)
> if(alpm_trans_addtarget(targ) == -1) {
> fprintf(stderr, _("error: '%s': %s\n"),
> targ, alpm_strerrorlast());
> - upgrade_cleanup();
> + trans_release();
> return(1);
> }
> }
> @@ -151,7 +129,7 @@ int pacman_upgrade(alpm_list_t *targets)
> default:
> break;
> }
> - upgrade_cleanup();
> + trans_release();
> FREELIST(data);
> return(1);
> }
> @@ -160,11 +138,13 @@ int pacman_upgrade(alpm_list_t *targets)
> /* Step 3: perform the installation */
> if(alpm_trans_commit(NULL) == -1) {
> fprintf(stderr, _("error: failed to commit transaction (%s)\n"), alpm_strerrorlast());
> - upgrade_cleanup();
> + trans_release();
> return(1);
> }
>
> - retval = upgrade_cleanup();
> + if(trans_release() == -1) {
> + retval = 1;
> + }
> return(retval);
> }
>
> diff --git a/src/pacman/util.c b/src/pacman/util.c
> index ed7669a..2cef54f 100644
> --- a/src/pacman/util.c
> +++ b/src/pacman/util.c
> @@ -42,6 +42,34 @@
> /* pacman */
> #include "util.h"
> #include "conf.h"
> +#include "callback.h"
> +
> +
> +int trans_init(pmtranstype_t type, pmtransflag_t flags)
> +{
> + if(alpm_trans_init(type, flags, cb_trans_evt,
> + cb_trans_conv, cb_trans_progress) == -1) {
> + /* TODO: error messages should be in the front end, not the back */
This comment is a bit stale, we can probably remove it. Printing error
messages in the backend is wrong, but having a message callback is
fine. I think this one is my fault a while back.
> + fprintf(stderr, _("error: failed to init transaction (%s)\n"),
> + alpm_strerrorlast());
> + if(pm_errno == PM_ERR_HANDLE_LOCK) {
> + fprintf(stderr, _(" if you're sure a package manager is not already\n"
> + " running, you can remove %s\n"), alpm_option_get_lockfile());
> + }
> + return(-1);
> + }
> + return(0);
> +}
> +
> +int trans_release()
> +{
> + if(alpm_trans_release() == -1) {
> + fprintf(stderr, _("error: failed to release transaction (%s)\n"),
> + alpm_strerrorlast());
> + return(-1);
> + }
> + return(0);
> +}
>
> int needs_transaction()
> {
> diff --git a/src/pacman/util.h b/src/pacman/util.h
> index f2facbf..722e4ab 100644
> --- a/src/pacman/util.h
> +++ b/src/pacman/util.h
> @@ -36,6 +36,8 @@
> /* update speed for the fill_progress based functions */
> #define UPDATE_SPEED_SEC 0.2f
>
> +int trans_init(pmtranstype_t type, pmtransflag_t flags);
> +int trans_release();
> int needs_transaction();
> int getcols();
> int makepath(const char *path);
> --
> 1.5.5.1
>
>
> _______________________________________________
> pacman-dev mailing list
> pacman-dev at archlinux.org
> http://archlinux.org/mailman/listinfo/pacman-dev
>
More information about the pacman-dev
mailing list