On Tue, Jan 5, 2010 at 9:11 AM, Nate Gallaher <[email protected]> wrote: > James Hawkins wrote: >> >> On Mon, Jan 4, 2010 at 7:05 AM, Nate Gallaher <[email protected]> >> wrote: >> >>> >>> James Hawkins wrote: >>> >>>> >>>> On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher >>>> <[email protected]> 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
