Re: [PATCH 1/2] msi: queue dynamically allocated strings in cond.y
On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher <ngallaher(a)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. -- James Hawkins
James Hawkins wrote:
On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher <ngallaher(a)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.
On Mon, Jan 4, 2010 at 7:05 AM, Nate Gallaher <ngallaher(a)deepthought.org> wrote:
James Hawkins wrote:
On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher <ngallaher(a)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. -- James Hawkins
James Hawkins wrote:
On Mon, Jan 4, 2010 at 7:05 AM, Nate Gallaher <ngallaher(a)deepthought.org> wrote:
James Hawkins wrote:
On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher <ngallaher(a)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? ~Nate
On Tue, Jan 5, 2010 at 9:11 AM, Nate Gallaher <ngallaher(a)deepthought.org> wrote:
James Hawkins wrote:
On Mon, Jan 4, 2010 at 7:05 AM, Nate Gallaher <ngallaher(a)deepthought.org> wrote:
James Hawkins wrote:
On Sat, Jan 2, 2010 at 10:36 AM, Nathan Gallaher <ngallaher(a)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
participants (2)
-
James Hawkins -
Nate Gallaher