[libvirt] [PATCH 0/1] Variable length structure allocator

I've had this patch hanging around for a while, and I think it's worth committing even though the original reason for it went away. The kind of structure it allocates is reasonably common, and the oversize calculation is tricky to get right. Since we've already done the work (thanks Jim for the oversize calculation) I think it's worth keeping. Dave David Allan (1): Implement variable length structure allocator src/util/memory.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/util/memory.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 0 deletions(-)

* This patch implements a memory allocator to obtain memory for structures whose last member is a variable length array. C99 refers to these variable length objects as structs containing flexible array members. --- src/util/memory.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/util/memory.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 0 deletions(-) diff --git a/src/util/memory.c b/src/util/memory.c index b2ee376..dd1216b 100644 --- a/src/util/memory.c +++ b/src/util/memory.c @@ -165,6 +165,46 @@ int virReallocN(void *ptrptr, size_t size, size_t count) } /** + * Vir_Alloc_Var: + * @ptrptr: pointer to hold address of allocated memory + * @struct_size: size of initial struct + * @element_size: size of array elements + * @count: number of array elements to allocate + * + * Allocate struct_size bytes plus an array of 'count' elements, each + * of size element_size. This sort of allocation is useful for + * receiving the data of certain ioctls and other APIs which return a + * struct in which the last element is an array of undefined length. + * The caller of this type of API is expected to know the length of + * the array that will be returned and allocate a suitable buffer to + * contain the returned data. C99 refers to these variable length + * objects as structs containing flexible array members. + * + * Returns -1 on failure, 0 on success + */ +int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) +{ + size_t alloc_size = 0; + +#if TEST_OOM + if (virAllocTestFail()) + return -1; +#endif + + if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { + errno = ENOMEM; + return -1; + } + + alloc_size = struct_size + (element_size * count); + *(void **)ptrptr = calloc(1, alloc_size); + if (*(void **)ptrptr == NULL) + return -1; + return 0; +} + + +/** * virFree: * @ptrptr: pointer to pointer for address of memory to be freed * diff --git a/src/util/memory.h b/src/util/memory.h index 0228173..c717610 100644 --- a/src/util/memory.h +++ b/src/util/memory.h @@ -48,6 +48,10 @@ 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; +int virAllocVar(void *ptrptr, + size_t struct_size, + size_t element_size, + size_t count) ATTRIBUTE_RETURN_CHECK; void virFree(void *ptrptr); /** @@ -88,6 +92,37 @@ void virFree(void *ptrptr); */ # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) +/* + * VIR_ALLOC_VAR_OVERSIZED: + * @M: size of base structure + * @N: number of array elements in trailing array + * @S: size of trailing array elements + * + * Check to make sure that the requested allocation will not cause + * arithmetic overflow in the allocation size. The check is + * essentially the same as that in gnulib's xalloc_oversized. + */ +#define VIR_ALLOC_VAR_OVERSIZED(M, N, S) ((((size_t)-1) - (M)) / (S) < (N)) + +/** + * VIR_ALLOC_VAR: + * @ptr: pointer to hold address of allocated memory + * @type: element type of trailing array + * @count: number of array elements to allocate + * + * Allocate sizeof(*ptr) bytes plus an array of 'count' elements, each + * sizeof('type'). This sort of allocation is useful for receiving + * the data of certain ioctls and other APIs which return a struct in + * which the last element is an array of undefined length. The caller + * of this type of API is expected to know the length of the array + * that will be returned and allocate a suitable buffer to contain the + * returned data. C99 refers to these variable length objects as + * structs containing flexible array members. + + * Returns -1 on failure, 0 on success + */ +#define VIR_ALLOC_VAR(ptr, type, count) virAllocVar(&(ptr), sizeof(*ptr), sizeof(type), (count)) + /** * VIR_FREE: * @ptr: pointer holding address to be freed -- 1.7.0.1

On 04/05/2010 11:21 AM, David Allan wrote:
+int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count)
Shouldn't that be void **ptrptr - that is, the caller passes in the address of a void* that we then modify?
+{ + size_t alloc_size = 0; + +#if TEST_OOM + if (virAllocTestFail()) + return -1; +#endif + + if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { + errno = ENOMEM; + return -1; + } + + alloc_size = struct_size + (element_size * count); + *(void **)ptrptr = calloc(1, alloc_size); + if (*(void **)ptrptr == NULL)
Especially since typing it correctly to begin with would avoid these ugly type-punning casts?
+++ b/src/util/memory.h @@ -48,6 +48,10 @@ 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; +int virAllocVar(void *ptrptr, + size_t struct_size, + size_t element_size, + size_t count) ATTRIBUTE_RETURN_CHECK;
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
+#define VIR_ALLOC_VAR(ptr, type, count) virAllocVar(&(ptr), sizeof(*ptr), sizeof(type), (count))
Should that second argument be sizeof(*(ptr)) for safety? On the other hand, the parenthesis around count are redundant, if you want to strip them. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, Apr 05, 2010 at 11:51:48AM -0600, Eric Blake wrote:
On 04/05/2010 11:21 AM, David Allan wrote:
+int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count)
Shouldn't that be void **ptrptr - that is, the caller passes in the address of a void* that we then modify?
+{ + size_t alloc_size = 0; + +#if TEST_OOM + if (virAllocTestFail()) + return -1; +#endif + + if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { + errno = ENOMEM; + return -1; + } + + alloc_size = struct_size + (element_size * count); + *(void **)ptrptr = calloc(1, alloc_size); + if (*(void **)ptrptr == NULL)
Especially since typing it correctly to begin with would avoid these ugly type-punning casts?
+++ b/src/util/memory.h @@ -48,6 +48,10 @@ 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; +int virAllocVar(void *ptrptr, + size_t struct_size, + size_t element_size, + size_t count) ATTRIBUTE_RETURN_CHECK;
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
I did originally try void** in these methods, but it didn't work out nicely, requiring extra casts in the macros. IIRC, the problem was that if you had char *foo; VIR_ALLOC_N(foo) then virAllocN would be given 'char **', which is compatible with void* but is not compatible with void**, without doing an manual cast. So switching these from void* to void** just moves where we need todo the fugly casts & I preferred them hidden in the memory.c impl rather than the header. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 04/05/2010 01:51 PM, Eric Blake wrote:
On 04/05/2010 11:21 AM, David Allan wrote:
+int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count)
Shouldn't that be void **ptrptr - that is, the caller passes in the address of a void* that we then modify?
+{ + size_t alloc_size = 0; + +#if TEST_OOM + if (virAllocTestFail()) + return -1; +#endif + + if (VIR_ALLOC_VAR_OVERSIZED(struct_size, count, element_size)) { + errno = ENOMEM; + return -1; + } + + alloc_size = struct_size + (element_size * count); + *(void **)ptrptr = calloc(1, alloc_size); + if (*(void **)ptrptr == NULL)
Especially since typing it correctly to begin with would avoid these ugly type-punning casts?
+++ b/src/util/memory.h @@ -48,6 +48,10 @@ 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; +int virAllocVar(void *ptrptr, + size_t struct_size, + size_t element_size, + size_t count) ATTRIBUTE_RETURN_CHECK;
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
+#define VIR_ALLOC_VAR(ptr, type, count) virAllocVar(&(ptr), sizeof(*ptr), sizeof(type), (count))
Should that second argument be sizeof(*(ptr)) for safety? On the other hand, the parenthesis around count are redundant, if you want to strip them.
Right, thanks; I'll fix that. Is everybody ok with having this allocator, btw? Dave

On 04/06/2010 06:59 AM, Dave Allan wrote:
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
Daniel's arguments are convincing; it's okay to keep the ugly cast in the implementation if it makes the header easier to use without having to embed a cast in the #define.
Is everybody ok with having this allocator, btw?
You have my ACK, but I'm only a (small) fraction of everybody. But since no one is using them yet, is it worth waiting until after 0.8.0 to push? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 04/07/2010 10:17 AM, Eric Blake wrote:
On 04/06/2010 06:59 AM, Dave Allan wrote:
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
Daniel's arguments are convincing; it's okay to keep the ugly cast in the implementation if it makes the header easier to use without having to embed a cast in the #define.
Agreed.
Is everybody ok with having this allocator, btw?
You have my ACK, but I'm only a (small) fraction of everybody. But since no one is using them yet, is it worth waiting until after 0.8.0 to push?
Waiting is a good idea; the patch has been hanging around my development box for weeks, so there's no hurry. Dave

On Wed, Apr 07, 2010 at 12:49:46PM -0400, Dave Allan wrote:
On 04/07/2010 10:17 AM, Eric Blake wrote:
On 04/06/2010 06:59 AM, Dave Allan wrote:
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
Daniel's arguments are convincing; it's okay to keep the ugly cast in the implementation if it makes the header easier to use without having to embed a cast in the #define.
Agreed.
Is everybody ok with having this allocator, btw?
You have my ACK, but I'm only a (small) fraction of everybody. But since no one is using them yet, is it worth waiting until after 0.8.0 to push?
Waiting is a good idea; the patch has been hanging around my development box for weeks, so there's no hurry.
Dave
Can I get another ack for this patch now that 0.8.0 is out? Dave

On Wed, Apr 14, 2010 at 01:31:15PM -0400, Dave Allan wrote:
On Wed, Apr 07, 2010 at 12:49:46PM -0400, Dave Allan wrote:
On 04/07/2010 10:17 AM, Eric Blake wrote:
On 04/06/2010 06:59 AM, Dave Allan wrote:
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
Daniel's arguments are convincing; it's okay to keep the ugly cast in the implementation if it makes the header easier to use without having to embed a cast in the #define.
Agreed.
Is everybody ok with having this allocator, btw?
You have my ACK, but I'm only a (small) fraction of everybody. But since no one is using them yet, is it worth waiting until after 0.8.0 to push?
Waiting is a good idea; the patch has been hanging around my development box for weeks, so there's no hurry.
Dave
Can I get another ack for this patch now that 0.8.0 is out?
ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Apr 14, 2010 at 06:37:35PM +0100, Daniel P. Berrange wrote:
On Wed, Apr 14, 2010 at 01:31:15PM -0400, Dave Allan wrote:
On Wed, Apr 07, 2010 at 12:49:46PM -0400, Dave Allan wrote:
On 04/07/2010 10:17 AM, Eric Blake wrote:
On 04/06/2010 06:59 AM, Dave Allan wrote:
Then again, fixing the type for your new method would imply fixing the typing of virAlloc and friends as well.
Daniel's arguments are convincing; it's okay to keep the ugly cast in the implementation if it makes the header easier to use without having to embed a cast in the #define.
Agreed.
Is everybody ok with having this allocator, btw?
You have my ACK, but I'm only a (small) fraction of everybody. But since no one is using them yet, is it worth waiting until after 0.8.0 to push?
Waiting is a good idea; the patch has been hanging around my development box for weeks, so there's no hurry.
Dave
Can I get another ack for this patch now that 0.8.0 is out?
ACK
Daniel
Ok, thanks. Pushed. Dave
participants (4)
-
Daniel P. Berrange
-
Dave Allan
-
David Allan
-
Eric Blake