[pacman-dev] [PATCH] lib/util: call _alpm_log before setting handle->pm_errno

[Dan McGee]

On Sat, Jun 18, 2011 at 6:26 PM, Dave Reisner <d at falconindy.com> wrote:
> This is an unfortunate chain of events. RET_ERR and RET_ERR_VOID will
> eventually call CHECK_HANDLE, which resets the handle's pm_errno member.
> Dan probably had a reason for doing this, so we merely switch the order
> of operations in the RET_ERR macros to avoid stomping on our pm_errno.
>
> Signed-off-by: Dave Reisner <d at falconindy.com>
> ---
> Awesome bug. Here's what happens when the DB is locked:
>
> $ sudo pacman -Sy
> :: Synchronizing package databases...
> error: failed to init transaction (unexpected error)
>
> I'm still not sure why CHECK_HANDLE will always reset handle->pm_errno. Seems
> like it's some sort of lazy initialization that should be getting done
> elsewhere.
It's not so much lazy init, it is required initialization we haven't
been doing. The misleading thing may be the CHECK_HANDLE() naming but
I couldn't think of anything better that wasn't unwieldy like
RESET_ERRNO_AND_ENSURE_HANDLE_EXISTS(). I thought I explained it in my
commit message:
http://projects.archlinux.org/pacman.git/commit/?id=ee015f086f3c40390659bbc0129b7c08ffd0ed5f

Basically the library was in a position to trip itself up all too
easily if a pm_errno value came in and persisted itself through an
operation that may need to look at that value.

> There may be a better way to solve this, if the reset isn't actually
> necessary.
We did talk about this on IRC but wanted to at least address it here.
This does fix it but we are probably better off not calling a public
API method to get the callback at all since we are just an internal
logger method.

>  lib/libalpm/util.h |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> index 0549c81..778e20f 100644
> --- a/lib/libalpm/util.h
> +++ b/lib/libalpm/util.h
> @@ -61,12 +61,14 @@
>
>  #define ASSERT(cond, action) do { if(!(cond)) { action; } } while(0)
>
> -#define RET_ERR_VOID(handle, err) do { (handle)->pm_errno = (err); \
> +#define RET_ERR_VOID(handle, err) do { \
>        _alpm_log(handle, PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerror(err)); \
> +       (handle)->pm_errno = (err); \
>        return; } while(0)
>
> -#define RET_ERR(handle, err, ret) do { (handle)->pm_errno = (err); \
> +#define RET_ERR(handle, err, ret) do { \
>        _alpm_log(handle, PM_LOG_DEBUG, "returning error %d from %s : %s\n", err, __func__, alpm_strerror(err)); \
> +       (handle)->pm_errno = (err); \
>        return (ret); } while(0)
>
>  #define DOUBLE_EQ(x, y) (fabs((x) - (y)) < DBL_EPSILON)
> --
> 1.7.5.4
>
>
>