[PATCH 1/2] msi: queue dynamically allocated strings in cond.y

[James Hawkins]

On Tue, Jan 5, 2010 at 9:11 AM, Nate Gallaher <ngallaher at deepthought.org> wrote:
> James Hawkins wrote:
>>
>> On Mon, Jan 4, 2010 at 7:05 AM, Nate Gallaher <ngallaher at deepthought.org>
>> wrote:
>>
>>>
>>> James Hawkins wrote:
>>>
>>>>
>>>> On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher
>>>> <ngallaher at deepthought.org> wrote:
>>>>
>>>>
>>>>>
>>>>>
>>>>
>>>> +struct cond_mem {
>>>> +    struct list entry;
>>>> +    void *ptr;
>>>> +};
>>>>
>>>>
>>>> +
>>>> +static void cond_free( void *info, void *ptr )
>>>> +{
>>>> +    COND_input *cond = (COND_input*) info;
>>>> +    struct cond_mem *mem, *safety;
>>>> +
>>>> +    LIST_FOR_EACH_ENTRY_SAFE( mem, safety, &cond->mem, struct cond_mem,
>>>> entry )
>>>> +    {
>>>> +        if( mem->ptr == ptr )
>>>> +        {
>>>> +            msi_free( mem->ptr );
>>>> +            list_remove( &(mem->entry) );
>>>> +            msi_free( mem );
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +    ERR("Error freeing %p\n", ptr);
>>>> +}
>>>>
>>>>
>>>> This won't fly.  cond_free needs to be an O(1) operation, like your
>>>> original patch.
>>>>
>>>>
>>>>
>>>
>>> This was exactly the same as your proposed implementation.
>>>
>>> Well, the simplest thing to do here is to no-op the cond_free function
>>> and
>>> just free all memory when parsing is done, but you seemed to not like
>>> that
>>> the last time.  Perhaps you were just looking for a solution that keeps
>>> the
>>> notion of a cond_free intact, with the implementation open?
>>>
>>> The only other alternative that I see here is to use a hashmap, but I
>>> don't
>>> immediately see an implementation of a proper hashmap available in
>>> include/wine.  I could go implement a hashmap, but I want to make sure
>>> that's the approach that you will accept before I do a bunch of work that
>>> you're not happy with again.
>>>
>>> Is there anything else that you talked with AJ about that I should know
>>> before I go further?  It would have been nice if whatever conversation
>>> there
>>> was had been shared publicly on the wine-devel list, since I could have
>>> avoided this headache.
>>>
>>>
>>
>> My solution was wrong, which is why it wasn't committed.  The correct
>> solution is a mix between our two original patches.  You allocate the
>> list entry and the string data in one chunk of memory that you can
>> free all at once (your original patch did this).  In cond_free, you
>> remove the entry from the list and free the allocation.  At the end of
>> parsing, you loop through the list, and preferable throw a WARN up
>> that we've leaked each allocation that is left.  Ideally, this only
>> happens when the parse fails and the WARN can be ignored.  Keep in
>> mind you have to add the data from msi_dup_property to the allocation
>> list.
>>
>>
>
> I am not seeing a way to do this which incorporates the ability to track
> externally allocated memory (like that of msi_dup_property). How do you
> propose to avoid a lookup during cond_free to check if the pointer given is
> from an external allocation?
>

Implement cond_add_allocation which takes the COND_info, the
allocation, and the size of that allocation.  You can either use that
size to call cond_aloc, copy over the old bits and free the old
allocation, or you can call msi_realloc on the old allocation adding
enough space for the list entry, which saves a copy, and add that list
entry to the allocation list.

-- 
James Hawkins