On Fri, May 30, 2008 at 03:37:06PM +0100, Daniel P. Berrange wrote:
> THis patch switches over the remote daemon to use the memory allocation
> APIs. This required exporting the 4 apis in the .so. I discovered along
> the way that none of the remote RPC dispatcher impls checked for malloc
> failure, so I've had to add that in too.
Looks fine, this really cleans things up, especially reallocs
> if (getsockname(sock->fd, (struct sockaddr *)(&sa), &salen) < 0)
> - return -1;
> + goto cleanup;
>
Hum, changes the semantic but it looks like it will avoid leaking
file descriptors too..
[..]
> +
> +cleanup:
> + for (i = 0; i < nfds; ++i)
> + close(fds[0]);
> + return -1;
[...]
> - if (ret->freeMems.freeMems_len == 0)
> + if (ret->freeMems.freeMems_len == 0) {
> + VIR_FREE(ret->freeMems.freeMems_val);
> return -1;
> + }
and memleaks ...
> diff -r 06acc2c5c1fb src/memory.c
> --- a/src/memory.c Thu May 29 16:05:55 2008 -0400
> +++ b/src/memory.c Fri May 30 10:36:42 2008 -0400
> @@ -104,7 +104,7 @@
> *
> * Returns -1 on failure to allocate, zero on success
> */
> -int virAlloc(void *ptrptr, size_t size)
> +int __virAlloc(void *ptrptr, size_t size)
> {
> #if TEST_OOM
> if (virAllocTestFail()) {
> @@ -137,7 +137,7 @@
> *
> * Returns -1 on failure to allocate, zero on success
> */
> -int virAllocN(void *ptrptr, size_t size, size_t count)
> +int __virAllocN(void *ptrptr, size_t size, size_t count)
> {
> #if TEST_OOM
> if (virAllocTestFail()) {
> @@ -171,7 +171,7 @@
> *
> * Returns -1 on failure to allocate, zero on success
> */
> -int virReallocN(void *ptrptr, size_t size, size_t count)
> +int __virReallocN(void *ptrptr, size_t size, size_t count)
> {
> void *tmp;
> #if TEST_OOM
> @@ -203,7 +203,7 @@
> * the 'ptrptr' variable. After release, 'ptrptr' will be
> * updated to point to NULL.
> */
> -void virFree(void *ptrptr)
> +void __virFree(void *ptrptr)
> {
> free(*(void**)ptrptr);
> *(void**)ptrptr = NULL;
> diff -r 06acc2c5c1fb src/memory.h
> --- a/src/memory.h Thu May 29 16:05:55 2008 -0400
> +++ b/src/memory.h Fri May 30 10:36:42 2008 -0400
> @@ -26,10 +26,10 @@
> #include "internal.h"
>
> /* Don't call these directly - use the macros below */
> -int virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
> -int virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> -int virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> -void virFree(void *ptrptr);
> +int __virAlloc(void *ptrptr, size_t size) ATTRIBUTE_RETURN_CHECK;
> +int __virAllocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> +int __virReallocN(void *ptrptr, size_t size, size_t count) ATTRIBUTE_RETURN_CHECK;
> +void __virFree(void *ptrptr);
>
> /**
> * VIR_ALLOC:
> @@ -41,7 +41,7 @@
> *
> * Returns -1 on failure, 0 on success
> */
> -#define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr)))
> +#define VIR_ALLOC(ptr) __virAlloc(&(ptr), sizeof(*(ptr)))
>
> /**
> * VIR_ALLOC_N:
> @@ -54,7 +54,7 @@
> *
> * Returns -1 on failure, 0 on success
> */
> -#define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count))
> +#define VIR_ALLOC_N(ptr, count) __virAllocN(&(ptr), sizeof(*(ptr)), (count))
>
> /**
> * VIR_REALLOC_N:
> @@ -67,7 +67,7 @@
> *
> * Returns -1 on failure, 0 on success
> */
> -#define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count))
> +#define VIR_REALLOC_N(ptr, count) __virReallocN(&(ptr), sizeof(*(ptr)), (count))
>
> /**
> * VIR_FREE:
> @@ -76,7 +76,7 @@
> * Free the memory stored in 'ptr' and update to point
> * to NULL.
> */
> -#define VIR_FREE(ptr) virFree(&(ptr));
> +#define VIR_FREE(ptr) __virFree(&(ptr))
ah, right we need to export those symbols now ...
looks good to me, +1
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list