[pacman-dev] BUG and PATCH: 'handle' not null after alpm_release()

[Markus Meissner]

On 01/19/2010 01:34 PM, Xavier Chantry wrote:
> On Tue, Jan 19, 2010 at 12:37 PM, Markus Meissner <markus at evigo.net> wrote:
>> Hi pacman dev list,
>>
>> I am the guy behind pyalpmm and I found a not so small bug in libalpm.
>>
>> As I wrote some unit-tests for pyalpmm I needed to open and close a
>> libalpm-"session" multiple times without restarting the calling
>> application. Something like that:
>>
>> 1. alpm_initialize();
>> 2. /** do some operation here **/
>> 3. alpm_release();
>> 4. alpm_initialize();
>> 5. alpm_release();
>>
>> But I couldn't do this, because the alpm_initialize() call in line 4
>> always threw an error (errno: PM_ERR_HANDLE_NOT_NULL), which translates
>> to: "library already initialized". This is obviously not true, because
>> the alpm_release() function was successfully called.
>>
>>
>> I checked the code and found out that this line threw the error:
>>
>> ASSERT(handle != NULL, RET_ERR(PM_ERR_HANDLE_NULL, -1));
>>
>> What means that the handle is not correctly reseted to NULL, but the
>> _alpm_free_handle() function, which is called by alpm_release() has a
>> FREE(handle) inside. So here is my explanation why this happens:
>>
>> The handle pointer is passed to _alpm_free_handle() as a value, so the
>> pointer value itself is copied. This works perfect for freeing the
>> contents of the handle struct, BUT setting this handle pointer to NULL
>> just sets the locally copied pointer to NULL, what means the original
>> handle variable remains un-touched. So every alpm_initialize() call
>> after the first one will throw this error, no matter if you called
>> alpm_release() or not.
>>
>> So to get finally end, I fixed it and the patch is (against latest git)
>> attached to this email. Hopefully my explanation is enough to sketch the
>> problem. Solving it, is really easy, just change the _alpm_free_handle()
>> function to take a pointer of a pointer of pmhandle_t as an argument,
>> change the references to handle inside the function and slightly change
>> the _alpm_free_handle() call in alpm_release(). That's how it is done
>> with the attached patch...
>>
> 
> What about just doing handle = NULL after the _alpm_free_handle() call ?

Yes also thought about that, but I don't like scattering such things
around. I mean, _alpm_free_handle() should do what it's called like. So
you also don't have to look at different places, if you want to know
what gets freed.

But honestly I don't mind a lot how, as long as this will be fixed at
some point in maybe near future, because it undermines the way I like to
write unit tests ;)

> 
>> Finally I also have a question: I saw many changes in the recent git
>> compared to the latest release. Is there an estimated release date?
>>
> 
> Not afaik. There was a plan to include some features (mainly GPG) for
> the next release that no one is working on.
> I wonder if we should not release anyway, because there are already
> some nice changes.

I would absolutly second a release, this would help me a lot as the API
changed at some points. It also would be incredibly great, if the API
changes would be a little more "third-party-library" friendly. Like
first marking items as deprecated for one minor release, before removing
them. I think there are some people working with libalpm who would
appreciate this, too - thinking about Dario Freddi for example. (Or am I
just too inattentive/slow and there was a deprecation-time for things
like alpm_trans_addtarget or the ALPM_TRANS_TYPEs?)

greets
Markus